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

JiaLiPassion · kara · commit 4374931b0ed3 · 2020-04-07T15:22:16.000-07:00
`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. PR Close #36022
diff --git a/packages/zone.js/lib/jest/jest.ts b/packages/zone.js/lib/jest/jest.ts
@@ -43,10 +43,9 @@ Zone.__load_patch('jest', (context: any, Zone: ZoneType) => {
 
   function wrapTestFactoryInZone(originalJestFn: Function) {
     return function(this: unknown, ...tableArgs: any[]) {
-      const testFn = originalJestFn.apply(this, tableArgs);
       return function(this: unknown, ...args: any[]) {
         args[1] = wrapTestInZone(args[1]);
-        return testFn.apply(this, args);
+        return originalJestFn.apply(this, tableArgs).apply(this, args);
       };
     };
   }
@@ -64,16 +63,21 @@ Zone.__load_patch('jest', (context: any, Zone: ZoneType) => {
   /**
    * Gets a function wrapping the body of a jest `it/beforeEach/afterEach` block to
    * execute in a ProxyZone zone.
-   * This will run in the `testProxyZone`.
+   * This will run in the `proxyZone`.
    */
   function wrapTestInZone(testBody: Function): Function {
     if (typeof testBody !== 'function') {
       return testBody;
     }
-    // The `done` callback is only passed through if the function expects at least one argument.
-    // Note we have to make a function with correct number of arguments, otherwise jest will
-    // think that all functions are sync or async.
-    return function(this: unknown, ...args: any[]) { return proxyZone.run(testBody, this, args); };
+    const wrappedFunc = function() {
+      return proxyZone.run(testBody, null, arguments as any);
+    };
+    // Update the length of wrappedFunc to be the same as the length of the testBody
+    // So jest core can handle whether the test function has `done()` or not correctly
+    Object.defineProperty(
+        wrappedFunc, 'length', {configurable: true, writable: true, enumerable: false});
+    wrappedFunc.length = testBody.length;
+    return wrappedFunc;
   }
 
   ['describe', 'xdescribe', 'fdescribe'].forEach(methodName => {
diff --git a/packages/zone.js/test/jest/jest.spec.js b/packages/zone.js/test/jest/jest.spec.js
@@ -6,34 +6,97 @@ function assertInsideSyncDescribeZone() {
 }
 describe('describe', () => {
   assertInsideSyncDescribeZone();
-  beforeEach(() => { assertInsideProxyZone(); });
-  beforeAll(() => { assertInsideProxyZone(); });
-  afterEach(() => { assertInsideProxyZone(); });
-  afterAll(() => { assertInsideProxyZone(); });
+  beforeEach(() => {
+    assertInsideProxyZone();
+  });
+  beforeAll(() => {
+    assertInsideProxyZone();
+  });
+  afterEach(() => {
+    assertInsideProxyZone();
+  });
+  afterAll(() => {
+    assertInsideProxyZone();
+  });
 });
 describe.each([[1, 2]])('describe.each', (arg1, arg2) => {
   assertInsideSyncDescribeZone();
   expect(arg1).toBe(1);
   expect(arg2).toBe(2);
 });
 describe('test', () => {
-  it('it', () => { assertInsideProxyZone(); });
+  it('it', () => {
+    assertInsideProxyZone();
+  });
   it.each([[1, 2]])('it.each', (arg1, arg2) => {
     assertInsideProxyZone();
     expect(arg1).toBe(1);
     expect(arg2).toBe(2);
   });
-  test('test', () => { assertInsideProxyZone(); });
-  test.each([[]])('test.each', () => { assertInsideProxyZone(); });
+  test('test', () => {
+    assertInsideProxyZone();
+  });
+  test.each([[]])('test.each', () => {
+    assertInsideProxyZone();
+  });
+});
+
+it('it', () => {
+  assertInsideProxyZone();
+});
+it('it with done', done => {
+  assertInsideProxyZone();
+  done();
 });
 
-it('it', () => { assertInsideProxyZone(); });
-it.each([[1, 2]])('it.each', (arg1, arg2) => {
+it.each([[1, 2]])('it.each', (arg1, arg2, done) => {
   assertInsideProxyZone();
   expect(arg1).toBe(1);
   expect(arg2).toBe(2);
+  done();
+});
+
+it.each([2])('it.each with 1D array', arg1 => {
+  assertInsideProxyZone();
+  expect(arg1).toBe(2);
+});
+
+it.each([2])('it.each with 1D array and done', (arg1, done) => {
+  assertInsideProxyZone();
+  expect(arg1).toBe(2);
+  done();
+});
+
+it.each`
+    foo  | bar
+    ${1} | ${2}
+  `('it.each should work with table as a tagged template literal', ({foo, bar}) => {
+  expect(foo).toBe(1);
+  expect(bar).toBe(2);
+});
+
+it.each`
+    foo  | bar
+    ${1} | ${2}
+  `('it.each should work with table as a tagged template literal with done', ({foo, bar}, done) => {
+  expect(foo).toBe(1);
+  expect(bar).toBe(2);
+  done();
+});
+
+it.each`
+    foo  | bar
+    ${1} | ${2}
+  `('(async) it.each should work with table as a tagged template literal', async ({foo, bar}) => {
+  expect(foo).toBe(1);
+  expect(bar).toBe(2);
+});
+
+test('test', () => {
+  assertInsideProxyZone();
+});
+test.each([[]])('test.each', () => {
+  assertInsideProxyZone();
 });
-test('test', () => { assertInsideProxyZone(); });
-test.each([[]])('test.each', () => { assertInsideProxyZone(); });
 
 test.todo('todo');
