Skip to content

fix: zone.js patch jest should handle done correctly#357

Merged
wtho merged 2 commits intothymikee:masterfrom
JiaLiPassion:zone-quick-fix
Mar 12, 2020
Merged

fix: zone.js patch jest should handle done correctly#357
wtho merged 2 commits intothymikee:masterfrom
JiaLiPassion:zone-quick-fix

Conversation

@JiaLiPassion
Copy link
Copy Markdown
Contributor

in the pervious PR #340, zone.js add support to 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 changed from

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

to

 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
previous logic

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

will not work for test.each.

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.

Close #356

Comment thread src/zone-patch/index.js Outdated
in the pervious PR thymikee#340, zone.js add support to 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 changed from

```
return testBody.length === 0
   ? () => testProxyZone.run(testBody, null)
   : done => testProxyZone.run(testBody, null, [done]);
```
to
```
 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
previous logic

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

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.
@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Mar 12, 2020

I will also do extra tests with this change today against my work projects’ tests.

@wtho
Copy link
Copy Markdown
Collaborator

wtho commented Mar 12, 2020

@ahnpnl ok, as soon as you give your thumbs up I will merge and release this fix.

Comment thread src/zone-patch/index.js
if (testBody.length > 0 || (eachArgs.length > 0 && eachArgs[0].length > 0)) {
eachArgs.forEach(row => {
const modifiedRow = row.map(a => {
a.push(testBody);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Error: TypeError: a.push is not a function. The example test

describe(`that is already cancelled`, () => {
    test.each(BASE_NOTIFICATION_DATA_SET)(`should compile and render`, notificationCompData => {
       prepareNotificationTest({
          cancelled: true,
          removedForThisUser: false,
       }, notificationCompData);

       prepareNotificationAssertion(notificationCompData);
    });
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ahnpnl, thanks for the test, could you show the content of the BASE_NOTIFICATION_DATA_SET?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here it is, it is a simple array of objects.

const BASE_NOTIFICATION_DATA_SET = [
      {
        isInbox: true,
        isHandset: true,
        isInforGo: true,
      },
      {
        isInbox: true,
        isHandset: true,
        isInforGo: false,
      },
];

Do you also need the content of the 2 functions prepareNotificationTest and prepareNotificationAssertion ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the current data is enough, thanks, will commit a fix.

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Mar 12, 2020

except the error I just commented, all async errors I saw from 8.1.0 were solved

@JiaLiPassion
Copy link
Copy Markdown
Contributor Author

@ahnpnl , I have updated the PR, and now the case should work, thanks.

@ahnpnl
Copy link
Copy Markdown
Collaborator

ahnpnl commented Mar 12, 2020

thanks a lot @JiaLiPassion , all work perfectly 👍 I think we release this fix @wtho

@wtho wtho merged commit 34287f5 into thymikee:master Mar 12, 2020
@wtho
Copy link
Copy Markdown
Collaborator

wtho commented Mar 12, 2020

I will do the release after work 👍 thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Getting "done is not a function" errors when updating to 8.1.0

3 participants