test(docs-infra): add missing tests for observables and promises by sonukapoor · Pull Request #34537 · angular/angular

sonukapoor · 2019-12-22T23:27:35Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #34398

Does this PR introduce a breaking change?

Yes
No

gkalpak · 2019-12-23T08:15:26Z

Marking this as blocked on #34327, since it builds on top of that.

sonukapoor · 2020-01-22T01:52:59Z

Marking this as blocked on #34327, since it builds on top of that.

@gkalpak This is unblocked now since #34327 has been merged. Can you review it please?

gkalpak

Thx, @sonukapoor ❤️
I've left some comments. Also, the current commit message ("tests for observables and promises") is too generic. Can you, please, change it to be more specific. E.g. something like:

test(docs-infra): add tests for `comparing-observables` example

gkalpak · 2020-01-23T17:42:52Z

I, also, just realized that we need to make sure the tests are run on CI (currently they are not afaict) 😁

gkalpak

Just few more (minor) comments to go 😃

Also, as mentioned in #34537 (comment), we need to run these tests on CI. This is similar to what you did for other Observables-related examples in #34063. I.e. you need to:

Add a dummy e2e/src/app.e2e-spec.ts (e.g. similar to observables/e2e/src/app.e2e-spec.ts).
Add an appropriate example-config.json (e.g. similar to observables/example-config.json but specifying a different command that actually runs the unit tests).

sonukapoor · 2020-01-25T20:04:07Z

Just few more (minor) comments to go 😃

Also, as mentioned in #34537 (comment), we need to run these tests on CI. This is similar to what you did for other Observables-related examples in #34063. I.e. you need to:

Add a dummy e2e/src/app.e2e-spec.ts (e.g. similar to observables/e2e/src/app.e2e-spec.ts).

Add an appropriate example-config.json (e.g. similar to observables/example-config.json but specifying a different command that actually runs the unit tests).

Yes, I haven't gotten to that yet. I wanted to make sure that we are happy with the codebase now. I should be able to look into the testing from CI next.

mary-poppins · 2020-01-26T00:24:40Z

You can preview a5231d7 at https://pr34537-a5231d7.ngbuilds.io/.

mary-poppins · 2020-01-26T00:50:27Z

You can preview d2bd1cc at https://pr34537-d2bd1cc.ngbuilds.io/.

mary-poppins · 2020-08-14T12:46:20Z

You can preview 23959b7 at https://pr34537-23959b7.ngbuilds.io/.

gkalpak

The tests look fine (with some minor comments). Thx for sorting this out.
I am not a big fan of the current setup. I'll look into whether it is possible to simplify it a bit.

gkalpak · 2020-08-22T10:16:44Z

@sonukapoor, I rebased on latest master and pushed a fixup commit that simplifies the setup: 6579810

BTW, I believe all changes belong to a single commit. Having one commit to add the tests and another one to run them doesn't make sense imo.

mary-poppins · 2020-08-22T10:25:50Z

You can preview 9683390 at https://pr34537-9683390.ngbuilds.io/.

sonukapoor · 2020-08-22T14:53:31Z

@sonukapoor, I rebased on latest master and pushed a fixup commit that simplifies the setup: 6579810

BTW, I believe all changes belong to a single commit. Having one commit to add the tests and another one to run them doesn't make sense imo.

Thanks @gkalpak - I have addressed the additional changes. I created the additional commit to separate between the setup to run the tests and to create the tests itself. I think they are both two different things. I can squash everything together if that is preferred.

mary-poppins · 2020-08-22T15:03:17Z

You can preview 470cef8 at https://pr34537-470cef8.ngbuilds.io/.

mary-poppins · 2020-08-22T15:47:40Z

You can preview 82d200a at https://pr34537-82d200a.ngbuilds.io/.

mary-poppins · 2020-08-24T15:08:45Z

You can preview d84dcc1 at https://pr34537-d84dcc1.ngbuilds.io/.

mary-poppins · 2020-08-24T15:51:37Z

You can preview d4da011 at https://pr34537-d4da011.ngbuilds.io/.

mary-poppins · 2020-08-26T10:08:01Z

You can preview 157a0f8 at https://pr34537-157a0f8.ngbuilds.io/.

gkalpak

The changes lgtm. Thx for the patience while we were figuring things out, @sonukapoor 😁 💯

One last thing:
The docs(docs-infra) commit message looks wrong. It should either be squashed into the first commit or it should be a test(docs-infra) commit.

sonukapoor · 2020-08-27T12:12:38Z

The changes lgtm. Thx for the patience while we were figuring things out, @sonukapoor 😁 💯

No worries @gkalpak

One last thing:
The docs(docs-infra) commit message looks wrong. It should either be squashed into the first commit or it should be a test(docs-infra) commit.

I have changed this squashed them. Please review one last time. Thanks

mary-poppins · 2020-08-27T12:19:53Z

You can preview f3bd82e at https://pr34537-f3bd82e.ngbuilds.io/.

mary-poppins · 2020-08-27T12:36:23Z

You can preview c265a4f at https://pr34537-c265a4f.ngbuilds.io/.

This commit adds missing tests for obserables and promises which are both stand-alone mini-apps.

This commit adds the necessary custom commands to run the tests in a node environment.

mary-poppins · 2020-08-27T14:29:33Z

You can preview e25f686 at https://pr34537-e25f686.ngbuilds.io/.

mary-poppins · 2020-08-27T14:42:11Z

You can preview ec9406f at https://pr34537-ec9406f.ngbuilds.io/.

gkalpak

Nice job, @sonukapoor! Thx 👍

Reviewed-for: global-docs-approvers

angular-automatic-lock-bot · 2020-09-27T16:20:56Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

sonukapoor requested a review from IgorMinar as a code owner December 22, 2019 23:27

sonukapoor requested a review from a team December 22, 2019 23:27

googlebot added the cla: yes label Dec 22, 2019

gkalpak added comp: docs-infra action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked target: patch This PR is targeted for the next patch release labels Dec 23, 2019

ngbot Bot modified the milestone: needsTriage Dec 23, 2019

sonukapoor force-pushed the tests-for-observables branch from 3eb2e21 to 583ad7f Compare January 22, 2020 01:50

sonukapoor requested a review from a team January 22, 2020 01:50

sonukapoor force-pushed the tests-for-observables branch from 583ad7f to aa9a17b Compare January 22, 2020 02:12

gkalpak changed the title ~~Tests for observables~~ test(docs-infra): add missing tests for obserables and promises Jan 22, 2020

gkalpak removed the state: blocked label Jan 22, 2020

gkalpak suggested changes Jan 22, 2020

View reviewed changes

gkalpak added the aio: preview label Jan 24, 2020

gkalpak suggested changes Jan 25, 2020

View reviewed changes

sonukapoor force-pushed the tests-for-observables branch from f450f03 to d902c0d Compare January 25, 2020 22:47

sonukapoor requested a review from a team January 25, 2020 22:47

sonukapoor force-pushed the tests-for-observables branch 4 times, most recently from 232d2e0 to a5231d7 Compare January 26, 2020 00:14

sonukapoor force-pushed the tests-for-observables branch from d2bd1cc to 98a1e73 Compare January 26, 2020 01:37

pullapprove Bot removed the request for review from IgorMinar July 23, 2020 03:33

sonukapoor force-pushed the tests-for-observables branch 2 times, most recently from 490eec2 to 9f71b16 Compare August 14, 2020 11:57

gkalpak suggested changes Aug 22, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

gkalpak reviewed Aug 24, 2020

View reviewed changes

Comment thread aio/content/examples/comparing-observables/src/promises.ts Resolved

gkalpak suggested changes Aug 24, 2020

View reviewed changes

Comment thread aio/content/examples/comparing-observables/src/promises.ts Resolved

gkalpak suggested changes Aug 27, 2020

View reviewed changes

sonukapoor added 2 commits August 27, 2020 10:18

test(docs-infra): add missing tests for obserables and promises

cc8ad7b

This commit adds missing tests for obserables and promises which are both stand-alone mini-apps.

test(docs-infra): add commands to run the unit tests

e25f686

This commit adds the necessary custom commands to run the tests in a node environment.

gkalpak suggested changes Aug 27, 2020

View reviewed changes

Comment thread aio/content/examples/comparing-observables/src/promises.spec.ts Resolved

fixup! test(docs-infra): add missing tests for obserables and promises

ec9406f

gkalpak approved these changes Aug 27, 2020

View reviewed changes

Conversation

sonukapoor commented Dec 22, 2019

PR Checklist

PR Type

What is the current behavior?

Does this PR introduce a breaking change?

Uh oh!

gkalpak commented Dec 23, 2019

Uh oh!

sonukapoor commented Jan 22, 2020

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gkalpak commented Jan 23, 2020

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonukapoor commented Jan 25, 2020

Uh oh!

mary-poppins commented Jan 26, 2020

Uh oh!

mary-poppins commented Jan 26, 2020

Uh oh!

mary-poppins commented Aug 14, 2020

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gkalpak commented Aug 22, 2020

Uh oh!

mary-poppins commented Aug 22, 2020

Uh oh!

sonukapoor commented Aug 22, 2020

Uh oh!

mary-poppins commented Aug 22, 2020

Uh oh!

mary-poppins commented Aug 22, 2020

Uh oh!

Uh oh!

mary-poppins commented Aug 24, 2020

Uh oh!

mary-poppins commented Aug 24, 2020

Uh oh!

Uh oh!

mary-poppins commented Aug 26, 2020

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

sonukapoor commented Aug 27, 2020

Uh oh!

mary-poppins commented Aug 27, 2020

Uh oh!

mary-poppins commented Aug 27, 2020