Skip to content

feat: support jest#35080

Closed
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:zone-jest
Closed

feat: support jest#35080
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:zone-jest

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

@JiaLiPassion JiaLiPassion commented Jan 31, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

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:

add basic support for jest, basically the functionality is the same as https://github.com/thymikee/jest-preset-angular/tree/master/src/zone-patch, and I move it here to easily share function with jasmine/mocha patch, and also I would like to add support for jest faketimer integration with fakeAsync. So jest-preset-angular can only care about the jest part.
I have discussed with the owner of jest-preset-angular in this PR thymikee/jest-preset-angular#340.

This PR will add the basic support, and I will add the fakeTimer support in the following up PRs.

@JiaLiPassion JiaLiPassion requested a review from mhevery January 31, 2020 15:06
@JiaLiPassion JiaLiPassion force-pushed the zone-jest branch 2 times, most recently from 4b48127 to 147c4fe Compare January 31, 2020 15:08
mhevery
mhevery suggested changes Jan 31, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can we add some tests to show that this works?
  • This needs some documentations so that our users can discover and use the feature.

@mhevery mhevery added the area: zones Issues related to zone.js label Feb 4, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Feb 4, 2020
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

JiaLiPassion commented Feb 5, 2020

@mhevery, Thanks for the review.

  1. I added the test cases for zone jest patch.
  2. I also tested this patch integration with angular in some sample app, because to integrate with angular, we still need jest-preset-angular, so I need this PR to be merged first, then I will go to
    the jest-preset-angular repo to remove zone-patch there. After that, I will add angular sample app test in angular repo.
  3. because the above reason (need to also update jest-preset-angular), I will update the doc after that.

@JiaLiPassion JiaLiPassion force-pushed the zone-jest branch 4 times, most recently from 69a0fbc to a029326 Compare February 5, 2020 13:44
mhevery
mhevery approved these changes Feb 5, 2020
View reviewed changes
Comment thread packages/zone.js/lib/jasmine/jasmine.ts Outdated
Comment thread packages/zone.js/lib/jest/jest.ts Outdated
Comment thread packages/zone.js/lib/jest/jest.ts Outdated
Copy link
Copy Markdown
Contributor

@mhevery mhevery Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Note we have to make a function with correct number of arguments, otherwise jasmine will
// Note we have to make a function with correct number of arguments, otherwise jest will

Copy link
Copy Markdown
Contributor Author

@JiaLiPassion JiaLiPassion Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks.

@mhevery mhevery added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Feb 5, 2020
@ngbot
Copy link
Copy Markdown

ngbot Bot commented Feb 5, 2020

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "ci/circleci: legacy-unit-tests-saucelabs" is failing
    failure status "ci/circleci: saucelabs_ivy" is failing
    pending forbidden labels detected: PR action: cleanup
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@JiaLiPassion JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 5, 2020
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Feb 6, 2020

presubmit

@kara kara closed this in daac33c Feb 7, 2020
@tonivj5 tonivj5 mentioned this pull request Feb 25, 2020
Merged
@AndrewKushnir AndrewKushnir mentioned this pull request Mar 3, 2020
Closed
3 tasks
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Mar 9, 2020

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Mar 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mhevery mhevery mhevery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

3 participants

@JiaLiPassion @mhevery @googlebot