test: never use subscribe with done callbacks in jasmine

Using subscribe with done callbacks in Jasmine leads to `afterEach` being called before the observable is unsubscribed from.

This in turn leads to teardown logic inside observables to only happen after `afterEach` runs.

This can be problematic when the teardown logic in observables interacts with logic in `afterEach`.

For instance, for webpack builders in watch mode, `afterEach` deletes source file folder while the teardown logic closes file watchers. Trying to delete files while they are still being watched leads to errors on Windows.

Author: filipesilva

packages/angular_devkit/architect/src/architect_spec.ts

@@ -61,7 +61,7 @@ describe('Architect', () => {
   beforeAll((done) => workspace.loadWorkspaceFromJson(workspaceJson).pipe(
     concatMap(ws => new Architect(ws).loadArchitect()),
     tap(arch => architect = arch),
-  ).subscribe(undefined, done.fail, done));
+  ).toPromise().then(done, done.fail));
 
   it('works', () => {
     const targetSpec = { project: 'app', target: 'browser', configuration: 'prod' };
@@ -101,24 +101,24 @@ describe('Architect', () => {
         expect(events[1].success).toBe(false);
         expect(events[2].success).toBe(true);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   });
 
   it('errors when builder cannot be resolved', (done) => {
     const targetSpec = { project: 'app', target: 'karma' };
     const builderConfig = architect.getBuilderConfiguration<BrowserTargetOptions>(targetSpec);
-    architect.run(builderConfig).subscribe(undefined, (err: Error) => {
+    architect.run(builderConfig).toPromise().then(() => done.fail(), (err: Error) => {
       expect(err).toEqual(jasmine.any(BuilderCannotBeResolvedException));
       done();
-    }, done.fail);
+    });
   });
 
   it('errors when builder options fail validation', (done) => {
     const targetSpec = { project: 'app', target: 'badBrowser' };
     const builderConfig = architect.getBuilderConfiguration<BrowserTargetOptions>(targetSpec);
-    architect.run(builderConfig).subscribe(undefined, (err: Error) => {
+    architect.run(builderConfig).toPromise().then(() => done.fail(), (err: Error) => {
       expect(err).toEqual(jasmine.any(schema.SchemaValidationException));
       done();
-    }, done.fail);
+    });
   });
 });

packages/angular_devkit/build_angular/test/app-shell/app-shell_spec_large.ts

@@ -11,8 +11,8 @@ import { Timeout, host, runTargetSpec } from '../utils';
 
 
 describe('AppShell Builder', () => {
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works (basic)', done => {
     host.replaceInFile('src/app/app.module.ts', /    BrowserModule/, `
@@ -44,7 +44,7 @@ describe('AppShell Builder', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
         expect(content).toMatch(/Welcome to app!/);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
 
   }, Timeout.Complex);
 });

packages/angular_devkit/build_angular/test/browser/allow-js_spec_large.ts

@@ -11,8 +11,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 
 
 describe('Browser Builder allow js', () => {
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     host.writeMultipleFiles({
@@ -25,7 +25,7 @@ describe('Browser Builder allow js', () => {
 
     runTargetSpec(host, browserTargetSpec).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 
   it('works with aot', (done) => {
@@ -38,6 +38,6 @@ describe('Browser Builder allow js', () => {
 
     runTargetSpec(host, browserTargetSpec, overrides).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 });

packages/angular_devkit/build_angular/test/browser/aot_spec_large.ts

@@ -14,8 +14,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 describe('Browser Builder AOT', () => {
   const outputPath = normalize('dist');
 
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     const overrides = { aot: true };
@@ -27,6 +27,6 @@ describe('Browser Builder AOT', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
         expect(content).toMatch(/platformBrowser.*bootstrapModuleFactory.*AppModuleNgFactory/);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Standard);
 });

packages/angular_devkit/build_angular/test/browser/assets_spec_large.ts

@@ -12,8 +12,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 
 
 describe('Browser Builder assets', () => {
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     const assets: { [path: string]: string } = {
@@ -55,7 +55,7 @@ describe('Browser Builder assets', () => {
         // .gitkeep should not be there.
         expect(host.scopedSync().exists(normalize('./dist/folder/.gitkeep'))).toBe(false);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 
   it('fails with non-absolute output path', (done) => {
@@ -100,6 +100,6 @@ describe('Browser Builder assets', () => {
     runTargetSpec(host, browserTargetSpec, overrides).pipe(
       toArray(),
       tap((buildEvents) => expect(buildEvents.length).toBe(1)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 });

packages/angular_devkit/build_angular/test/browser/base-href_spec_large.ts

@@ -14,8 +14,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 describe('Browser Builder base href', () => {
   const outputPath = normalize('dist');
 
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     host.writeMultipleFiles({
@@ -32,6 +32,6 @@ describe('Browser Builder base href', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(fileName));
         expect(content).toMatch(/<base href="\/myUrl">/);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 });

packages/angular_devkit/build_angular/test/browser/build-optimizer_spec_large.ts

@@ -14,8 +14,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 describe('Browser Builder build optimizer', () => {
   const outputPath = normalize('dist');
 
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     const overrides = { aot: true, buildOptimizer: true };
@@ -26,6 +26,6 @@ describe('Browser Builder build optimizer', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
         expect(content).not.toMatch(/\.decorators =/);
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 });

packages/angular_devkit/build_angular/test/browser/bundle-budgets_spec_large.ts

@@ -12,8 +12,8 @@ import { TestLogger, Timeout, browserTargetSpec, host, runTargetSpec } from '../
 
 describe('Browser Builder bundle budgets', () => {
 
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('accepts valid bundles', (done) => {
     const overrides = {
@@ -26,7 +26,7 @@ describe('Browser Builder bundle budgets', () => {
     runTargetSpec(host, browserTargetSpec, overrides, logger).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
       tap(() => expect(logger.includes('WARNING')).toBe(false)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Complex);
 
   it('shows errors', (done) => {
@@ -37,7 +37,7 @@ describe('Browser Builder bundle budgets', () => {
 
     runTargetSpec(host, browserTargetSpec, overrides).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(false)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Complex);
 
   it('shows warnings', (done) => {
@@ -51,6 +51,6 @@ describe('Browser Builder bundle budgets', () => {
     runTargetSpec(host, browserTargetSpec, overrides, logger).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
       tap(() => expect(logger.includes('WARNING')).toBe(true)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Complex);
 });

packages/angular_devkit/build_angular/test/browser/circular-dependency_spec_large.ts

@@ -11,8 +11,8 @@ import { TestLogger, Timeout, browserTargetSpec, host, runTargetSpec } from '../
 
 
 describe('Browser Builder circular dependency detection', () => {
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('works', (done) => {
     host.appendToFile('src/app/app.component.ts',
@@ -24,6 +24,6 @@ describe('Browser Builder circular dependency detection', () => {
     runTargetSpec(host, browserTargetSpec, overrides, logger).pipe(
       tap((buildEvent) => expect(buildEvent.success).toBe(true)),
       tap(() => expect(logger.includes('Circular dependency detected')).toBe(true)),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 });

packages/angular_devkit/build_angular/test/browser/deploy-url_spec_large.ts

@@ -14,8 +14,8 @@ import { Timeout, browserTargetSpec, host, runTargetSpec } from '../utils';
 describe('Browser Builder deploy url', () => {
   const outputPath = normalize('dist');
 
-  beforeEach(done => host.initialize().subscribe(undefined, done.fail, done));
-  afterEach(done => host.restore().subscribe(undefined, done.fail, done));
+  beforeEach(done => host.initialize().toPromise().then(done, done.fail));
+  afterEach(done => host.restore().toPromise().then(done, done.fail));
 
   it('uses deploy url for bundles urls', (done) => {
     const overrides = { deployUrl: 'deployUrl/' };
@@ -35,7 +35,7 @@ describe('Browser Builder deploy url', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
         expect(content).toContain('http://example.com/some/path/main.js');
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 
   it('uses deploy url for in webpack runtime', (done) => {
@@ -48,7 +48,7 @@ describe('Browser Builder deploy url', () => {
         const content = virtualFs.fileBufferToString(host.scopedSync().read(normalize(fileName)));
         expect(content).toContain('deployUrl/');
       }),
-    ).subscribe(undefined, done.fail, done);
+    ).toPromise().then(done, done.fail);
   }, Timeout.Basic);
 
 });