fix: test.each should be in ProxyZone #340
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice! Could you add a minimal test using @thymikee you definitely have to have a look at this, as you authored the patch for Do you plan to migrate all patches from |
|
@wtho, thanks for the review, sure I will add test under And thanks for the awesome work for |
thymikee
reviewed
Jan 22, 2020
src/zone-patch/index.js
Outdated
| ? () => testProxyZone.run(testBody, null) | ||
| : done => testProxyZone.run(testBody, null, [done]); | ||
| return function() { | ||
| return testProxyZone.run(testBody, null, arguments); |
There was a problem hiding this comment.
How about return (...args) => testProxyZone.run(testBody, null, args)? Not that it matters too much, it's just my preference of not using arguments when possible
There was a problem hiding this comment.
Sure, I also think that is a good idea, will update it.
| [2, { id: 2, name: 'Test Hero 2' }] | ||
| ])('should call the GET hero api and return the result', (id: number, testData: any) => { | ||
| debugger; | ||
| console.log('id, testData', id, testData, (window as any).Zone.current.name); |
There was a problem hiding this comment.
Sorry, forget to remove
|
@thymikee , I have updated the PR, also add test cases for |
JiaLiPassion
force-pushed
the
each
branch
2 times, most recently
from
2771812 to
68c42b1
Compare
12 tasks
|
Any news? I have seen that angular/angular#35080 was merged 18 days ago 🎉 |
wtho
reviewed
Feb 25, 2020
wtho
approved these changes
Feb 25, 2020
|
Can we merge this? @thymikee |
|
Hey guys, any news on this? We are eagerly waiting :) |
|
@mehrad-rafigh Sorry for the delay, it's out now. |
JiaLiPassion
added a commit
to JiaLiPassion/jest-preset-angular
that referenced
this pull request
Mar 11, 2020
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.
JiaLiPassion
added a commit
to JiaLiPassion/jest-preset-angular
that referenced
this pull request
Mar 11, 2020
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.
JiaLiPassion
added a commit
to JiaLiPassion/jest-preset-angular
that referenced
this pull request
Mar 11, 2020
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.
JiaLiPassion
added a commit
to JiaLiPassion/jest-preset-angular
that referenced
this pull request
Mar 11, 2020
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #339.
Fix
test.eachnot in ProxyZone issue.Also remove some not needed assignment.
And I will try to move this source into
angular/zone.jsrepo, and also add support to timer methods such asjest.fakeTimers/jest.runAllTicks, after thatjest-preset-angularcan just importzone-testingbundle.@thymikee , please help to check this one, thank you!