Skip to content

Commit

Permalink
fix: zone.js patch jest should handle done correctly
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
JiaLiPassion committed Mar 11, 2020
1 parent 1ae55c6 commit 8953020
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 5 deletions.
20 changes: 20 additions & 0 deletions example/src/app/app.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,26 @@ describe('AppComponent', () => {
done();
}, 100);
});

it('async with done should work', async done => {
let flag = false;
setTimeout(() => {
flag = true;
expect(flag).toBe(true);
done();
}, 100);
});

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

(it.each([[1, 2]]) as any)('it.each with done', (arg1, arg2, done) => {
expect(arg1).toBe(1);
expect(arg2).toBe(2);
done();
});
});

test.todo('a sample todo');
75 changes: 70 additions & 5 deletions src/zone-patch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,68 @@ function wrapDescribeInZone(describeBody) {
// Create a proxy zone in which to run `test` blocks so that the tests function
// can retroactively install different zones.
const testProxyZone = ambientZone.fork(new ProxyZoneSpec());
function wrapTestInZone(testBody) {
function wrapTestInZone(testBody, eachArgs) {
if (testBody === undefined) {
return;
}
return function(...args) {
return testProxyZone.run(testBody, null, args);
};

if (!eachArgs || eachArgs.length === 0 || eachArgs[0].length === 0) {
// If we are not handling `test.each`, then the parameter of `testBody`
// will be 0 or 1, if it is 1, then we need to return a function with
// done parameter
return testBody.length === 0
? () => testProxyZone.run(testBody, null)
: done => testProxyZone.run(testBody, null, [done]);
} else {
/*
If we are handling `test.each`, and the each arguments have data,
then we need to dynamically create a Function, the reason is we can
only know the parameters of `test.each` method at runtime
For example:
```
test.each([[1, 2]])('test.each', (arg1, arg2) => {});
```
In this case we need to return a function like this
```
return function(arg1, arg2) {
return testProxyZone.run(testBody, null, [arg1, arg2]);
}
```
The most important part is we have to explicitly return the function
with 2 parameters, we can not return something like this,
```
return function() {
return testProxyZone.run(testBody, null, arguments);
}
```
It looks the logic is the same, but the explicit one will tell jest
the number of the parameters, and jest can handle `done()` or without `done()`
correctly.
And also because we are using `new Function` syntax here, the function body can not
access local scope, so we can not use `testProxyZone` or `testBody` variable
directly, so we have to pass them as parameters.
*/
const len = eachArgs[0].length;
const args = [];
let argString = '';
for (let i = 0; i < len; i++) {
args.push('arg' + i);
argString += 'arg' + i;
if (i !== len - 1) {
argString += ',';
}
}
args.push('testBody');
args.push('testProxyZone');
if (len < testBody.length) {
args.push('done');
argString += ',done';
}
const funcBody = `
return testProxyZone.run(testBody, null, [${argString}])`;
return new Function(args, funcBody);
}
}

/**
Expand All @@ -70,7 +125,17 @@ const bindDescribe = originalJestFn =>
const bindTest = originalJestFn =>
function(...eachArgs) {
return function(...args) {
args[1] = wrapTestInZone(args[1]);
const testBody = args[1];
args[1] = wrapTestInZone(args[1], ...eachArgs);
if (testBody.length > 0 || (eachArgs.length > 0 && eachArgs[0].length > 0)) {
eachArgs.forEach(row => {
const modifiedRow = row.map(a => {
a.push(testBody);
a.push(testProxyZone);
});
return modifiedRow;
});
}
return originalJestFn.apply(this, eachArgs).apply(this, args);
};
};
Expand Down

0 comments on commit 8953020

Please sign in to comment.