Don't create spies in beforeAll.

Creating them in beforeAll will usually cause test contamination, so they should be created in beforeEach instead so they are reset for each test. Change-Id: I51f11de85555cbf5b8c18969eabeecbe8617b8f3
Pavelas · May 16, 2019 · a3be4d8 · a3be4d8
1 parent 7aae7ed
commit a3be4d8
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 36 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -138,6 +138,22 @@ module.exports = {
         'allowSamePrecedence': false,
       },
     ],
+    'no-restricted-syntax': [
+      'error',
+      {
+        'selector': 'CallExpression[callee.name="beforeAll"] ' +
+                    ':matches(' +
+                    'CallExpression[callee.property.name="createSpy"],' +
+                    'CallExpression[callee.name="spyOn"])',
+        'message': 'Create spies in beforeEach, not beforeAll.',
+      },
+      {
+        'selector': 'CallExpression' +
+                    '[callee.name=/^([fx]?it|(before|after)(Each|All))$/] > ' +
+                    ':function[async=true][params.length>0]',
+        'message': 'Don\'t use both async and done.',
+      },
+    ],
     'no-whitespace-before-property': 'error',
     'nonblock-statement-body-position': ['error', 'below'],
     'operator-assignment': 'error',

diff --git a/test/cast/cast_proxy_unit.js b/test/cast/cast_proxy_unit.js
@@ -33,27 +33,24 @@ describe('CastProxy', () => {
   /** @type {shaka.cast.CastProxy} */
   let proxy;
 
-  beforeAll(() => {
+  beforeEach(() => {
     mockCastSenderConstructor = jasmine.createSpy('CastSender constructor');
     mockCastSenderConstructor.and.callFake(createMockCastSender);
-
     shaka.cast.CastSender = Util.spyFunc(mockCastSenderConstructor);
-  });
 
-  afterAll(() => {
-    shaka.cast.CastSender = originalCastSender;
-  });
-
-  beforeEach(() => {
     mockVideo = new shaka.test.FakeVideo();
     mockPlayer = createMockPlayer();
     mockSender = null;
 
     proxy = new CastProxy(mockVideo, mockPlayer, fakeAppId);
   });
 
-  afterEach((done) => {
-    proxy.destroy().catch(fail).then(done);
+  afterEach(async () => {
+    try {
+      await proxy.destroy();
+    } finally {
+      shaka.cast.CastSender = originalCastSender;
+    }
   });
 
   describe('constructor', () => {

diff --git a/test/player_unit.js b/test/player_unit.js
@@ -61,20 +61,16 @@ describe('Player', () => {
   /** @type {!shaka.test.FakeVideo} */
   let video;
 
-  beforeAll(() => {
+  beforeEach(() => {
+    // By default, errors are a failure.
     logErrorSpy = jasmine.createSpy('shaka.log.error');
+    logErrorSpy.calls.reset();
     shaka.log.error = shaka.test.Util.spyFunc(logErrorSpy);
+
     logWarnSpy = jasmine.createSpy('shaka.log.warning');
+    logErrorSpy.and.callFake(fail);
     shaka.log.warning = shaka.test.Util.spyFunc(logWarnSpy);
     shaka.log.alwaysWarn = shaka.test.Util.spyFunc(logWarnSpy);
-  });
-
-  beforeEach(() => {
-    // By default, errors are a failure.
-    logErrorSpy.calls.reset();
-    logErrorSpy.and.callFake(fail);
-
-    logWarnSpy.calls.reset();
 
     // Since this is not an integration test, we don't want MediaSourceEngine to
     // fail assertions based on browser support for types.  Pretend that all
@@ -149,15 +145,18 @@ describe('Player', () => {
     player.addEventListener('error', shaka.test.Util.spyFunc(onError));
   });
 
-  afterEach((done) => {
-    player.destroy().catch(fail).then(done);
+  afterEach(async () => {
+    try {
+      await player.destroy();
+    } finally {
+      shaka.log.error = originalLogError;
+      shaka.log.warning = originalLogWarn;
+      shaka.log.alwaysWarn = originalLogAlwaysWarn;
+      window.MediaSource.isTypeSupported = originalIsTypeSupported;
+    }
   });
 
   afterAll(() => {
-    shaka.log.error = originalLogError;
-    shaka.log.warning = originalLogWarn;
-    shaka.log.alwaysWarn = originalLogAlwaysWarn;
-    window.MediaSource.isTypeSupported = originalIsTypeSupported;
   });
 
   describe('destroy', () => {

diff --git a/test/text/vtt_text_parser_unit.js b/test/text/vtt_text_parser_unit.js
@@ -18,17 +18,18 @@
 describe('VttTextParser', () => {
   const Cue = shaka.text.Cue;
   const CueRegion = shaka.text.CueRegion;
+  const originalLogWarning = shaka.log.warning;
 
   /** @type {!jasmine.Spy} */
   let logWarningSpy;
 
-  beforeAll(() => {
+  beforeEach(() => {
     logWarningSpy = jasmine.createSpy('shaka.log.warning');
     shaka.log.warning = shaka.test.Util.spyFunc(logWarningSpy);
   });
 
-  beforeEach(() => {
-    logWarningSpy.calls.reset();
+  afterEach(() => {
+    shaka.log.warning = originalLogWarning;
   });
 
   it('supports no cues', () => {

diff --git a/test/util/fake_event_target_unit.js b/test/util/fake_event_target_unit.js
@@ -24,19 +24,16 @@ describe('FakeEventTarget', () => {
   /** @type {!jasmine.Spy} */
   let logErrorSpy;
 
-  beforeAll(() => {
+  beforeEach(() => {
     logErrorSpy = jasmine.createSpy('shaka.log.error');
+    logErrorSpy.and.callFake(fail);
     shaka.log.error = Util.spyFunc(logErrorSpy);
-  });
 
-  afterAll(() => {
-    shaka.log.error = originalLogError;
+    target = new shaka.util.FakeEventTarget();
   });
 
-  beforeEach(() => {
-    target = new shaka.util.FakeEventTarget();
-    logErrorSpy.calls.reset();
-    logErrorSpy.and.callFake(fail);
+  afterEach(() => {
+    shaka.log.error = originalLogError;
   });
 
   it('sets target on dispatched events', (done) => {