From 8a5eb086726640ca150cf3477b1232fc3f8cace8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Fri, 19 Aug 2016 12:10:53 -0700 Subject: [PATCH] fix(fakeAsync): have fakeAsync use Proxy zone. (#10797) Closes #10503 It is possible for code in `beforeEach` to capture and fork a zone (for example creating `NgZone` in `beforeEach`). Subsequently the code in `it` may chose to do `fakeAsync`. The issue is that because the code in `it` can use `NgZone` from the `beforeEach`. it effectively can escape the `fakeAsync` zone. A solution is to run all of the test in `ProxyZone` which allows a test to dynamically replace the rules at any time. This allows the `beforeEach` to fork a zone, and then `it` to retroactively became `fakeAsync` zone. --- karma-js.conf.js | 2 + .../compiler-cli/integrationtest/test/init.ts | 3 + .../core/test/application_init_spec.ts | 4 +- modules/@angular/core/test/fake_async_spec.ts | 25 +++++++-- .../@angular/core/test/zone/ng_zone_spec.ts | 43 ++++++++++++++- modules/@angular/core/testing/async.ts | 55 +++++++++++++++++-- modules/@angular/core/testing/fake_async.ts | 33 +++++++---- .../@angular/core/testing/testing_internal.ts | 14 ++--- .../@angular/forms/test/form_group_spec.ts | 12 +++- modules/@angular/router/karma.conf.js | 2 + npm-shrinkwrap.clean.json | 2 +- npm-shrinkwrap.json | 9 +-- package.json | 2 +- tools/cjs-jasmine/index-tools.ts | 7 ++- tools/cjs-jasmine/index.ts | 9 ++- tools/types.d.ts | 1 + 16 files changed, 178 insertions(+), 45 deletions(-) diff --git a/karma-js.conf.js b/karma-js.conf.js index 47ebddc698..ee3ab95478 100644 --- a/karma-js.conf.js +++ b/karma-js.conf.js @@ -19,6 +19,8 @@ module.exports = function(config) { 'node_modules/zone.js/dist/zone.js', 'node_modules/zone.js/dist/long-stack-trace-zone.js', + 'node_modules/zone.js/dist/proxy-zone.js', + 'node_modules/zone.js/dist/sync-test.js', 'node_modules/zone.js/dist/jasmine-patch.js', 'node_modules/zone.js/dist/async-test.js', 'node_modules/zone.js/dist/fake-async-test.js', diff --git a/modules/@angular/compiler-cli/integrationtest/test/init.ts b/modules/@angular/compiler-cli/integrationtest/test/init.ts index 9834ae7648..927bdb8fd7 100644 --- a/modules/@angular/compiler-cli/integrationtest/test/init.ts +++ b/modules/@angular/compiler-cli/integrationtest/test/init.ts @@ -12,3 +12,6 @@ require('reflect-metadata'); require('zone.js/dist/zone-node.js'); require('zone.js/dist/long-stack-trace-zone.js'); +require('zone.js/dist/sync-test.js'); +require('zone.js/dist/proxy-zone.js'); +require('zone.js/dist/jasmine-patch.js'); diff --git a/modules/@angular/core/test/application_init_spec.ts b/modules/@angular/core/test/application_init_spec.ts index d60c4db087..baa7bb2fae 100644 --- a/modules/@angular/core/test/application_init_spec.ts +++ b/modules/@angular/core/test/application_init_spec.ts @@ -13,9 +13,9 @@ export function main() { describe('no initializers', () => { it('should return true for `done`', - inject([ApplicationInitStatus], (status: ApplicationInitStatus) => { + async(inject([ApplicationInitStatus], (status: ApplicationInitStatus) => { expect(status.done).toBe(true); - })); + }))); it('should return a promise that resolves immediately for `donePromise`', async(inject([ApplicationInitStatus], (status: ApplicationInitStatus) => { diff --git a/modules/@angular/core/test/fake_async_spec.ts b/modules/@angular/core/test/fake_async_spec.ts index 83cfeb04e8..39cf6a6ec6 100644 --- a/modules/@angular/core/test/fake_async_spec.ts +++ b/modules/@angular/core/test/fake_async_spec.ts @@ -14,6 +14,7 @@ import {expect} from '@angular/platform-browser/testing/matchers'; import {Parser} from '../../compiler/src/expression_parser/parser'; const resolvedPromise = Promise.resolve(null); +const ProxyZoneSpec: {assertPresent: () => void} = (Zone as any)['ProxyZoneSpec']; export function main() { describe('fake async', () => { @@ -294,11 +295,27 @@ export function main() { zoneInTest1 = Zone.current; expect(zoneInTest1).toBe(zoneInBeforeEach); })); + }); + }); - it('should use a different zone between tests', fakeAsync(() => { - expect(Zone.current).toBe(zoneInBeforeEach); - expect(Zone.current).not.toBe(zoneInTest1); - })); + describe('ProxyZone', () => { + beforeEach(() => { ProxyZoneSpec.assertPresent(); }); + + afterEach(() => { ProxyZoneSpec.assertPresent(); }); + + it('should allow fakeAsync zone to retroactively set a zoneSpec outside of fakeAsync', () => { + ProxyZoneSpec.assertPresent(); + var state: string = 'not run'; + const testZone = Zone.current.fork({name: 'test-zone'}); + (fakeAsync(() => { + testZone.run(() => { + Promise.resolve('works').then((v) => state = v); + expect(state).toEqual('not run'); + flushMicrotasks(); + expect(state).toEqual('works'); + }); + }))(); + expect(state).toEqual('works'); }); }); } diff --git a/modules/@angular/core/test/zone/ng_zone_spec.ts b/modules/@angular/core/test/zone/ng_zone_spec.ts index 5db6bdf91d..327081e9e4 100644 --- a/modules/@angular/core/test/zone/ng_zone_spec.ts +++ b/modules/@angular/core/test/zone/ng_zone_spec.ts @@ -6,11 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ +import {BaseException} from '@angular/core'; import {NgZone} from '@angular/core/src/zone/ng_zone'; +import {async, fakeAsync, flushMicrotasks} from '@angular/core/testing'; import {AsyncTestCompleter, Log, beforeEach, ddescribe, describe, expect, iit, inject, it, xdescribe, xit} from '@angular/core/testing/testing_internal'; import {browserDetection} from '@angular/platform-browser/testing/browser_util'; -import {BaseException} from '../../src/facade/exceptions'; import {isPresent, scheduleMicroTask} from '../../src/facade/lang'; var needsLongerTimers = browserDetection.isSlow || browserDetection.isEdge; @@ -753,4 +754,44 @@ function commonTests() { }, resultTimer); }), testTimeout); }); + + describe('bugs', () => { + describe('#10503', () => { + let ngZone: NgZone; + + beforeEach(inject([NgZone], (_ngZone: NgZone) => { + // Create a zone outside the fakeAsync. + ngZone = _ngZone; + })); + + it('should fakeAsync even if the NgZone was created outside.', fakeAsync(() => { + let result: string = null; + // try to escape the current fakeAsync zone by using NgZone which was created outside. + ngZone.run(() => { + Promise.resolve('works').then((v) => result = v); + flushMicrotasks(); + }); + expect(result).toEqual('works'); + })); + + describe('async', () => { + let asyncResult: string; + const waitLongerThenTestFrameworkAsyncTimeout = 5; + + beforeEach(() => { asyncResult = null; }); + + it('should async even if the NgZone was created outside.', async(() => { + // try to escape the current async zone by using NgZone which was created outside. + ngZone.run(() => { + setTimeout(() => { + Promise.resolve('works').then((v) => asyncResult = v); + }, waitLongerThenTestFrameworkAsyncTimeout); + }); + })); + + afterEach(() => { expect(asyncResult).toEqual('works'); }); + + }); + }); + }); } diff --git a/modules/@angular/core/testing/async.ts b/modules/@angular/core/testing/async.ts index 9b6cd960eb..b2e4befe01 100644 --- a/modules/@angular/core/testing/async.ts +++ b/modules/@angular/core/testing/async.ts @@ -32,7 +32,13 @@ export function async(fn: Function): (done: any) => any { // function when asynchronous activity is finished. if (_global.jasmine) { return (done: any) => { - runInTestZone(fn, done, (err: string | Error) => { + if (!done) { + // if we run beforeEach in @angular/core/testing/testing_internal then we get no done + // fake it here and assume sync. + done = function() {}; + done.fail = function(e: any) { throw e; }; + } + runInTestZone(fn, done, (err: any) => { if (typeof err === 'string') { return done.fail(new Error(err)); } else { @@ -50,13 +56,52 @@ export function async(fn: Function): (done: any) => any { } function runInTestZone(fn: Function, finishCallback: Function, failCallback: Function) { - var AsyncTestZoneSpec = (Zone as any /** TODO #9100 */)['AsyncTestZoneSpec']; + const currentZone = Zone.current; + var AsyncTestZoneSpec = (Zone as any)['AsyncTestZoneSpec']; if (AsyncTestZoneSpec === undefined) { throw new Error( 'AsyncTestZoneSpec is needed for the async() test helper but could not be found. ' + 'Please make sure that your environment includes zone.js/dist/async-test.js'); } - var testZoneSpec = new AsyncTestZoneSpec(finishCallback, failCallback, 'test'); - var testZone = Zone.current.fork(testZoneSpec); - return testZone.run(fn); + const ProxyZoneSpec = (Zone as any)['ProxyZoneSpec'] as { + get(): {setDelegate(spec: ZoneSpec): void; getDelegate(): ZoneSpec;}; + assertPresent: () => void; + }; + if (ProxyZoneSpec === undefined) { + throw new Error( + 'ProxyZoneSpec is needed for the async() test helper but could not be found. ' + + 'Please make sure that your environment includes zone.js/dist/proxy-zone.js'); + } + const proxyZoneSpec = ProxyZoneSpec.get(); + ProxyZoneSpec.assertPresent(); + // We need to create the AsyncTestZoneSpec outside the ProxyZone. + // If we do it in ProxyZone then we will get to infinite recursion. + const proxyZone = Zone.current.getZoneWith('ProxyZoneSpec'); + const previousDelegate = proxyZoneSpec.getDelegate(); + proxyZone.parent.run(() => { + var testZoneSpec: ZoneSpec = new AsyncTestZoneSpec( + () => { + // Need to restore the original zone. + currentZone.run(() => { + if (proxyZoneSpec.getDelegate() == testZoneSpec) { + // Only reset the zone spec if it's sill this one. Otherwise, assume it's OK. + proxyZoneSpec.setDelegate(previousDelegate); + } + finishCallback(); + }); + }, + (error: any) => { + // Need to restore the original zone. + currentZone.run(() => { + if (proxyZoneSpec.getDelegate() == testZoneSpec) { + // Only reset the zone spec if it's sill this one. Otherwise, assume it's OK. + proxyZoneSpec.setDelegate(previousDelegate); + } + failCallback(error); + }); + }, + 'test'); + proxyZoneSpec.setDelegate(testZoneSpec); + }); + return Zone.current.runGuarded(fn); } diff --git a/modules/@angular/core/testing/fake_async.ts b/modules/@angular/core/testing/fake_async.ts index 235285c782..924f7f08ff 100644 --- a/modules/@angular/core/testing/fake_async.ts +++ b/modules/@angular/core/testing/fake_async.ts @@ -8,9 +8,14 @@ import {BaseException} from '../index'; -let _FakeAsyncTestZoneSpecType = (Zone as any /** TODO #9100 */)['FakeAsyncTestZoneSpec']; -let _fakeAsyncZone: Zone = null; +const FakeAsyncTestZoneSpec = (Zone as any)['FakeAsyncTestZoneSpec']; +type ProxyZoneSpec = { + setDelegate(delegateSpec: ZoneSpec): void; getDelegate(): ZoneSpec; resetDelegate(): void; +}; +const ProxyZoneSpec: {get(): ProxyZoneSpec; assertPresent: () => ProxyZoneSpec} = + (Zone as any)['ProxyZoneSpec']; + let _fakeAsyncTestZoneSpec: any = null; /** @@ -20,8 +25,8 @@ let _fakeAsyncTestZoneSpec: any = null; * @experimental */ export function resetFakeAsyncZone() { - _fakeAsyncZone = null; _fakeAsyncTestZoneSpec = null; + ProxyZoneSpec.assertPresent().resetDelegate(); } let _inFakeAsyncCall = false; @@ -45,26 +50,30 @@ let _inFakeAsyncCall = false; * @experimental */ export function fakeAsync(fn: Function): (...args: any[]) => any { - return function(...args: any[] /** TODO #9100 */) { + return function(...args: any[]) { + const proxyZoneSpec = ProxyZoneSpec.assertPresent(); if (_inFakeAsyncCall) { throw new BaseException('fakeAsync() calls can not be nested'); } _inFakeAsyncCall = true; try { - if (!_fakeAsyncZone) { - if (Zone.current.get('FakeAsyncTestZoneSpec') != null) { + if (!_fakeAsyncTestZoneSpec) { + if (proxyZoneSpec.getDelegate() instanceof FakeAsyncTestZoneSpec) { throw new BaseException('fakeAsync() calls can not be nested'); } - _fakeAsyncTestZoneSpec = new _FakeAsyncTestZoneSpecType(); - _fakeAsyncZone = Zone.current.fork(_fakeAsyncTestZoneSpec); + _fakeAsyncTestZoneSpec = new FakeAsyncTestZoneSpec(); } - let res = _fakeAsyncZone.run(() => { - let res = fn(...args); + let res: any; + const lastProxyZoneSpec = proxyZoneSpec.getDelegate(); + proxyZoneSpec.setDelegate(_fakeAsyncTestZoneSpec); + try { + res = fn(...args); flushMicrotasks(); - return res; - }); + } finally { + proxyZoneSpec.setDelegate(lastProxyZoneSpec); + } if (_fakeAsyncTestZoneSpec.pendingPeriodicTimers.length > 0) { throw new BaseException( diff --git a/modules/@angular/core/testing/testing_internal.ts b/modules/@angular/core/testing/testing_internal.ts index 96246c1a5d..e3b5609cd4 100644 --- a/modules/@angular/core/testing/testing_internal.ts +++ b/modules/@angular/core/testing/testing_internal.ts @@ -34,7 +34,7 @@ var jsmIt = _global.it; var jsmIIt = _global.fit; var jsmXIt = _global.xit; -var runnerStack: any[] /** TODO #9100 */ = []; +var runnerStack: BeforeEachRunner[] = []; var inIt = false; jasmine.DEFAULT_TIMEOUT_INTERVAL = 3000; var globalTimeOut = jasmine.DEFAULT_TIMEOUT_INTERVAL; @@ -62,7 +62,7 @@ class BeforeEachRunner { // Reset the test providers before each test jsmBeforeEach(() => { testBed.resetTestingModule(); }); -function _describe(jsmFn: any /** TODO #9100 */, ...args: any[] /** TODO #9100 */) { +function _describe(jsmFn: Function, ...args: any[]) { var parentRunner = runnerStack.length === 0 ? null : runnerStack[runnerStack.length - 1]; var runner = new BeforeEachRunner(parentRunner); runnerStack.push(runner); @@ -71,15 +71,15 @@ function _describe(jsmFn: any /** TODO #9100 */, ...args: any[] /** TODO #9100 * return suite; } -export function describe(...args: any[] /** TODO #9100 */): void { +export function describe(...args: any[]): void { return _describe(jsmDescribe, ...args); } -export function ddescribe(...args: any[] /** TODO #9100 */): void { +export function ddescribe(...args: any[]): void { return _describe(jsmDDescribe, ...args); } -export function xdescribe(...args: any[] /** TODO #9100 */): void { +export function xdescribe(...args: any[]): void { return _describe(jsmXDescribe, ...args); } @@ -105,7 +105,7 @@ export function beforeEach(fn: Function): void { * {provide: SomeToken, useValue: myValue}, * ]); */ -export function beforeEachProviders(fn: any /** TODO #9100 */): void { +export function beforeEachProviders(fn: Function): void { jsmBeforeEach(() => { var providers = fn(); if (!providers) return; @@ -128,7 +128,7 @@ export function addProviders(providers: Array): void { /** * @deprecated */ -export function beforeEachBindings(fn: any /** TODO #9100 */): void { +export function beforeEachBindings(fn: Function): void { beforeEachProviders(fn); } diff --git a/modules/@angular/forms/test/form_group_spec.ts b/modules/@angular/forms/test/form_group_spec.ts index 43db1e69d0..affb21705d 100644 --- a/modules/@angular/forms/test/form_group_spec.ts +++ b/modules/@angular/forms/test/form_group_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {fakeAsync, tick} from '@angular/core/testing'; +import {async, fakeAsync, tick} from '@angular/core/testing'; import {AsyncTestCompleter, beforeEach, ddescribe, describe, iit, inject, it, xit} from '@angular/core/testing/testing_internal'; import {FormControl, FormGroup, Validators} from '@angular/forms'; @@ -624,8 +624,14 @@ export function main() { }); describe('statusChanges', () => { - const control = new FormControl('', asyncValidatorReturningObservable); - const group = new FormGroup({'one': control}); + let control: FormControl; + let group: FormGroup; + + beforeEach(async(() => { + control = new FormControl('', asyncValidatorReturningObservable); + group = new FormGroup({'one': control}); + })); + // TODO(kara): update these tests to use fake Async it('should fire a statusChange if child has async validation change', diff --git a/modules/@angular/router/karma.conf.js b/modules/@angular/router/karma.conf.js index 66ba3d0b56..09ce039071 100644 --- a/modules/@angular/router/karma.conf.js +++ b/modules/@angular/router/karma.conf.js @@ -21,6 +21,8 @@ module.exports = function(config) { // Zone.js dependencies 'node_modules/zone.js/dist/zone.js', 'node_modules/zone.js/dist/long-stack-trace-zone.js', + 'node_modules/zone.js/dist/proxy-zone.js', + 'node_modules/zone.js/dist/sync-test.js', 'node_modules/zone.js/dist/jasmine-patch.js', 'node_modules/zone.js/dist/async-test.js', 'node_modules/zone.js/dist/fake-async-test.js', diff --git a/npm-shrinkwrap.clean.json b/npm-shrinkwrap.clean.json index 41aaf25e57..98c455b08f 100644 --- a/npm-shrinkwrap.clean.json +++ b/npm-shrinkwrap.clean.json @@ -5774,7 +5774,7 @@ } }, "zone.js": { - "version": "0.6.12" + "version": "0.6.14" } }, "name": "angular-srcs", diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 01898aad43..27797d0c9d 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -2886,7 +2886,8 @@ }, "lodash": { "version": "4.14.2", - "from": "lodash@>=4.0.0 <5.0.0" + "from": "lodash@4.14.2", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.14.2.tgz" }, "readable-stream": { "version": "2.0.6", @@ -9213,9 +9214,9 @@ } }, "zone.js": { - "version": "0.6.12", - "from": "zone.js@0.6.12", - "resolved": "https://registry.npmjs.org/zone.js/-/zone.js-0.6.12.tgz" + "version": "0.6.14", + "from": "zone.js@0.6.14", + "resolved": "https://registry.npmjs.org/zone.js/-/zone.js-0.6.14.tgz" } } } diff --git a/package.json b/package.json index 6ac229353b..2e16fb42e1 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "es6-shim": "^0.35.0", "reflect-metadata": "^0.1.3", "rxjs": "5.0.0-beta.6", - "zone.js": "^0.6.12" + "zone.js": "^0.6.14" }, "devDependencies": { "@types/angularjs": "^1.5.13-alpha", diff --git a/tools/cjs-jasmine/index-tools.ts b/tools/cjs-jasmine/index-tools.ts index c3a66be05c..02c511c28f 100644 --- a/tools/cjs-jasmine/index-tools.ts +++ b/tools/cjs-jasmine/index-tools.ts @@ -1,15 +1,19 @@ 'use strict'; var glob = require('glob'); +require('zone.js/dist/zone-node.js'); var JasmineRunner = require('jasmine'); var path = require('path'); // require('es6-shim/es6-shim.js'); -require('zone.js/dist/zone-node.js'); require('zone.js/dist/long-stack-trace-zone.js'); +require('zone.js/dist/proxy-zone.js'); +require('zone.js/dist/sync-test.js'); require('zone.js/dist/async-test.js'); require('zone.js/dist/fake-async-test.js'); var jrunner = new JasmineRunner(); +(global as any)['jasmine'] = jrunner.jasmine; +require('zone.js/dist/jasmine-patch.js'); var toolsDir = process.cwd() + '/dist/tools'; function toolsDirRequire(moduleId: string) { return require(path.join(toolsDir, moduleId)); @@ -39,6 +43,5 @@ jrunner.configureDefaultReporter({showColors: process.argv.indexOf('--no-color') jrunner.onComplete(function(passed: boolean) { process.exit(passed ? 0 : 1); }); jrunner.projectBaseDir = path.resolve(__dirname, '../../'); jrunner.specDir = ''; -require('zone.js/dist/jasmine-patch.js'); specFiles.forEach((file: string) => { toolsDirRequire(file); }); jrunner.execute(); diff --git a/tools/cjs-jasmine/index.ts b/tools/cjs-jasmine/index.ts index 860697cb29..4f99868cdb 100644 --- a/tools/cjs-jasmine/index.ts +++ b/tools/cjs-jasmine/index.ts @@ -1,17 +1,21 @@ 'use strict'; var glob = require('glob'); +require('zone.js/dist/zone-node.js'); var JasmineRunner = require('jasmine'); var path = require('path'); require('source-map-support').install(); // require('es6-shim/es6-shim.js'); -require('zone.js/dist/zone-node.js'); require('zone.js/dist/long-stack-trace-zone.js'); +require('zone.js/dist/proxy-zone.js'); +require('zone.js/dist/sync-test.js'); require('zone.js/dist/async-test.js'); require('zone.js/dist/fake-async-test.js'); require('reflect-metadata/Reflect'); - var jrunner = new JasmineRunner(); +(global as any)['jasmine'] = jrunner.jasmine; +require('zone.js/dist/jasmine-patch.js'); + var distAll: string = process.cwd() + '/dist/all'; function distAllRequire(moduleId: string) { return require(path.join(distAll, moduleId)); @@ -66,7 +70,6 @@ jrunner.onComplete(function(passed: boolean) { process.exit(passed ? 0 : 1); }); jrunner.projectBaseDir = path.resolve(__dirname, '../../'); jrunner.specDir = ''; require('./test-cjs-main.js'); -require('zone.js/dist/jasmine-patch.js'); distAllRequire('@angular/platform-server/src/parse5_adapter.js').Parse5DomAdapter.makeCurrent(); specFiles.forEach((file: string) => { var r = distAllRequire(file); diff --git a/tools/types.d.ts b/tools/types.d.ts index f0d4642286..e4c8edfafa 100644 --- a/tools/types.d.ts +++ b/tools/types.d.ts @@ -2,3 +2,4 @@ /// /// +///