Skip to content

fix(zone.js): zone.js patch jest should handle done correctly#36022

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

fix(zone.js): zone.js patch jest should handle done correctly#36022
JiaLiPassion wants to merge 1 commit intoangular:masterfrom
JiaLiPassion:jest-fix

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

zone.js support jest test.each() methods, but it
introduces a bug, which is the done() function will not be handled correctly.

it('should work with done', done => {
  // done will be undefined.
});

The reason is the logic of monkey patching test method is different from jasmine patch

// jasmine patch

return testBody.length === 0
   ? () => testProxyZone.run(testBody, null)
   : done => testProxyZone.run(testBody, null, [done]);

// jest patch

 return function(...args) {
   return testProxyZone.run(testBody, null, args);
 };

the purpose of this change is to handle the following cases.

test.each([1, 2])('test.each', (arg1, arg2) => {
  expect(arg1).toBe(1);
  expect(arg2).toBe(2);
});

so in jest, it is a little complex, because the testBody's parameter may be bigger than 1, so the
logic in jasmine

return testBody.length === 0
   ? () => testProxyZone.run(testBody, null)
   : done => testProxyZone.run(testBody, null, [done]);

will not work for test.each in jest.

So in this PR, I created a dynamic Function to return the correct length of paramters (which is required by jest core), to handle

  1. normal test with or without done.
  2. each with parameters with or without done.

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:

What is the current behavior?

Issue Number: The issue is found at jest-preset-angular repo here, thymikee/jest-preset-angular#356

@JiaLiPassion JiaLiPassion requested a review from mhevery March 11, 2020 16:46
@JiaLiPassion JiaLiPassion added the area: zones Issues related to zone.js label Mar 11, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Mar 11, 2020
@JiaLiPassion JiaLiPassion added the target: major This PR is targeted for the next major release label Mar 12, 2020
@JiaLiPassion JiaLiPassion force-pushed the jest-fix branch 2 times, most recently from 5525d99 to 031652a Compare March 13, 2020 15:27
Comment thread packages/zone.js/lib/jest/jest.ts Outdated
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Mar 31, 2020
@kara kara added the action: presubmit The PR is in need of a google3 presubmit label Apr 2, 2020
@mhevery mhevery removed the action: presubmit The PR is in need of a google3 presubmit label Apr 6, 2020
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Apr 6, 2020

presubmit

Copy link
Copy Markdown
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Small grammatical changes

Can we also update the commit message to change 'support' to 'supports':

zone.js support jest test.each() methods => zone.js supports jest test.each() methods

Comment thread packages/zone.js/lib/jest/jest.ts Outdated
Comment thread packages/zone.js/lib/jest/jest.ts Outdated
@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 6, 2020
@JiaLiPassion JiaLiPassion requested a review from kara April 6, 2020 22:47
@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 Apr 6, 2020
@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@kara , thanks for the review, I just updated the comment and the commit message.

`zone.js` supports jest `test.each()` methods, but it
introduces a bug, which is the `done()` function will not be handled correctly.

```
it('should work with done', done => {
  // done will be undefined.
});
```

The reason is the logic of monkey patching `test` method is different from `jasmine` patch

// jasmine patch
```
return testBody.length === 0
   ? () => testProxyZone.run(testBody, null)
   : done => testProxyZone.run(testBody, null, [done]);
```

// jest patch
```
 return function(...args) {
   return testProxyZone.run(testBody, null, args);
 };
```

the purpose of this change is to handle the following cases.

```
test.each([1, 2])('test.each', (arg1, arg2) => {
  expect(arg1).toBe(1);
  expect(arg2).toBe(2);
});
```

so in jest, it is a little complex, because the `testBody`'s parameter may be bigger than 1, so the
logic in `jasmine`

```
return testBody.length === 0
   ? () => testProxyZone.run(testBody, null)
   : done => testProxyZone.run(testBody, null, [done]);
```
will not work for `test.each` in jest.

So in this PR, I created a dynamic `Function` to return the correct length of paramters (which is required by jest core), to handle
1. normal `test` with or without `done`.
2. each with parameters with or without done.
Copy link
Copy Markdown
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kara
Copy link
Copy Markdown
Contributor

kara commented Apr 7, 2020

new presubmit

@kara kara added action: presubmit The PR is in need of a google3 presubmit and removed action: presubmit The PR is in need of a google3 presubmit labels Apr 7, 2020
@kara kara closed this in 4374931 Apr 7, 2020
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 May 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

Development

Successfully merging this pull request may close these issues.

4 participants