Fix unit tests to ensure that test registrations happen before tests. (#1788)

Previously the test registries would happen as a function of imports. This doesn't ensure that unconstrained tests run before tests get registered.

This PR creates a `setup_test.ts` file like we do in WebGPU which imports the registries and then parses the karma flags. This ensures the expected order.

This PR also adds --testEnv as a command line flag instead of --backend.
DEV

Author: Nikhil Thorat

DEVELOPMENT.md

@@ -48,9 +48,10 @@ $ yarn test --single-run
 To run the tests in an environment that does not have GPU support (such as Chrome Remote Desktop):
 
 ```bash
-$ yarn test --backend cpu
+$ yarn test --testEnv cpu
 ```
 
+Available test environments: cpu, webgl1, webgl2.
 
 #### Packaging (browser and npm)
 

karma.conf.js

@@ -33,7 +33,7 @@ if (coverageEnabled) {
 
 const devConfig = {
   frameworks: ['jasmine', 'karma-typescript'],
-  files: [{pattern: 'src/**/*.ts'}],
+  files: ['src/setup_test.ts', {pattern: 'src/**/*.ts'}],
   exclude: [
     'src/test_node.ts',
     'src/backends/webgpu/**/*.ts',
@@ -46,7 +46,7 @@ const devConfig = {
 
 const browserstackConfig = {
   frameworks: ['browserify', 'jasmine'],
-  files: [{pattern: 'dist/**/*_test.js'}],
+  files: ['dist/setup_test.js', {pattern: 'dist/**/*_test.js'}],
   exclude: [
     'dist/test_node.js',
     'dist/test_async_backends.js',
@@ -60,8 +60,10 @@ const browserstackConfig = {
 
 module.exports = function(config) {
   const args = [];
-  if (config.backend) {
-    args.push('--backend', config.backend);
+  // If no test environment is set unit tests will run against all registered
+  // test environments.
+  if (config.testEnv) {
+    args.push('--testEnv', config.testEnv);
   }
   if (config.grep) {
     args.push('--grep', config.grep);

scripts/enumerate-tests.js

@@ -14,6 +14,11 @@
 // limitations under the License.
 // =============================================================================
 
+/**
+ * This script generates the tests.ts file which enumerates all the
+ * backend-agonstic tests. These are the tests that get executed from other
+ * packages (e.g. WebGPU).
+ */
 // Call this script from the root of the repo.
 
 const LICENSE = `/**
@@ -50,7 +55,8 @@ function findTestFiles(dir, files) {
         !file.startsWith('.') && fs.statSync(filePath).isDirectory() &&
         !fs.existsSync(path.join(filePath, 'package.json'))) {
       files = findTestFiles(filePath, files);
-    } else if (filePath.endsWith('_test.ts')) {
+    } else if (
+        filePath.endsWith('_test.ts') && filePath !== 'src/setup_test.ts') {
       files.push(filePath.replace('src/', './').replace('.ts', ''));
     }
   });

scripts/test-ci.sh

@@ -22,16 +22,16 @@ yarn test-node-ci
 
 # Run the first karma separately so it can download the BrowserStack binary
 # without conflicting with others.
-yarn run-browserstack --browsers=bs_safari_mac --backend webgl --flags '{"WEBGL_CPU_FORWARD": false, "WEBGL_SIZE_UPLOAD_UNIFORM": 0}'
+yarn run-browserstack --browsers=bs_safari_mac --testEnv webgl1 --flags '{"WEBGL_CPU_FORWARD": false, "WEBGL_SIZE_UPLOAD_UNIFORM": 0}'
 
 # Run the rest of the karma tests in parallel. These runs will reuse the
 # already downloaded binary.
 npm-run-all -p -c --aggregate-output \
-  "run-browserstack --browsers=bs_safari_mac --flags '{\"HAS_WEBGL\": false}' --backend cpu" \
-  "run-browserstack --browsers=win_10_chrome --backend webgl --flags '{\"WEBGL_CPU_FORWARD\": false, \"WEBGL_SIZE_UPLOAD_UNIFORM\": 0}'" \
-  "run-browserstack --browsers=bs_ios_11 --backend webgl --flags '{\"WEBGL_CPU_FORWARD\": false, \"WEBGL_SIZE_UPLOAD_UNIFORM\": 0}'" \
-  "run-browserstack --browsers=bs_ios_11 --flags '{\"HAS_WEBGL\": false}' --backend cpu" \
+  "run-browserstack --browsers=bs_safari_mac --flags '{\"HAS_WEBGL\": false}' --testEnv cpu" \
+  "run-browserstack --browsers=win_10_chrome --testEnv webgl2 --flags '{\"WEBGL_CPU_FORWARD\": false, \"WEBGL_SIZE_UPLOAD_UNIFORM\": 0}'" \
+  "run-browserstack --browsers=bs_ios_11 --testEnv webgl1 --flags '{\"WEBGL_CPU_FORWARD\": false, \"WEBGL_SIZE_UPLOAD_UNIFORM\": 0}'" \
+  "run-browserstack --browsers=bs_ios_11 --flags '{\"HAS_WEBGL\": false}' --testEnv cpu" \
   "run-browserstack --browsers=bs_firefox_mac" \
   "run-browserstack --browsers=bs_chrome_mac" \
-  "run-browserstack --browsers=bs_chrome_mac --backend webgl --flags '{\"WEBGL_CPU_FORWARD\": true}'" \
-  "run-browserstack --browsers=bs_chrome_mac --backend webgl --flags '{\"WEBGL_CPU_FORWARD\": false}'"
+  "run-browserstack --browsers=bs_chrome_mac --testEnv webgl2 --flags '{\"WEBGL_CPU_FORWARD\": true}'" \
+  "run-browserstack --browsers=bs_chrome_mac --testEnv webgl2 --flags '{\"WEBGL_CPU_FORWARD\": false}'"

src/jasmine_util.ts

@@ -61,45 +61,57 @@ export function envSatisfiesConstraints(
   return true;
 }
 
-// tslint:disable-next-line:no-any
-declare let __karma__: any;
-
-export function parseKarmaFlags(args: string[]): TestEnv {
+export function parseTestEnvFromKarmaFlags(
+    args: string[], registeredTestEnvs: TestEnv[]): TestEnv {
   let flags: Flags;
-  let factory: () => Promise<KernelBackend>| KernelBackend;
-  let backendName = '';
-  const backendNames = ENGINE.backendNames()
-                           .map(backendName => '\'' + backendName + '\'')
-                           .join(', ');
+  let testEnvName: string;
 
   args.forEach((arg, i) => {
     if (arg === '--flags') {
       flags = JSON.parse(args[i + 1]);
-    } else if (arg === '--backend') {
-      const type = args[i + 1];
-      backendName = type;
-      factory = ENGINE.findBackendFactory(backendName.toLowerCase());
-      if (factory == null) {
-        throw new Error(
-            `Unknown value ${type} for flag --backend. ` +
-            `Allowed values are ${backendNames}.`);
-      }
+    } else if (arg === '--testEnv') {
+      testEnvName = args[i + 1];
     }
   });
 
-  if (flags == null && factory == null) {
+  const testEnvNames = registeredTestEnvs.map(env => env.name).join(', ');
+  if (flags != null && testEnvName == null) {
+    throw new Error(
+        '--testEnv flag is required when --flags is present. ' +
+        `Available values are [${testEnvNames}].`);
+  }
+  if (testEnvName == null) {
     return null;
   }
-  if (flags != null && factory == null) {
+
+  let testEnv: TestEnv;
+  registeredTestEnvs.forEach(env => {
+    if (env.name === testEnvName) {
+      testEnv = env;
+    }
+  });
+  if (testEnv == null) {
     throw new Error(
-        '--backend flag is required when --flags is present. ' +
-        `Available values are ${backendNames}.`);
+        `Test environment with name ${testEnvName} not ` +
+        `found. Available test environment names are ` +
+        `${testEnvNames}`);
+  }
+  if (flags != null) {
+    testEnv.flags = flags;
   }
-  return {flags: flags || {}, name: backendName, backendName};
+
+  return testEnv;
 }
 
 export function describeWithFlags(
     name: string, constraints: Constraints, tests: (env: TestEnv) => void) {
+  if (TEST_ENVS.length === 0) {
+    throw new Error(
+        `Found no test environments. This is likely due to test environment ` +
+        `registries never being imported or test environment registries ` +
+        `being registered too late.`);
+  }
+
   TEST_ENVS.forEach(testEnv => {
     ENV.setFlags(testEnv.flags);
     if (envSatisfiesConstraints(ENV, testEnv, constraints)) {
@@ -138,13 +150,6 @@ export function registerTestEnv(testEnv: TestEnv) {
   TEST_ENVS.push(testEnv);
 }
 
-if (typeof __karma__ !== 'undefined') {
-  const testEnv = parseKarmaFlags(__karma__.config.args);
-  if (testEnv != null) {
-    setTestEnvs([testEnv]);
-  }
-}
-
 function executeTests(
     testName: string, tests: (env: TestEnv) => void, testEnv: TestEnv) {
   describe(testName, () => {

src/jasmine_util_test.ts

@@ -16,8 +16,7 @@
  */
 
 import {Environment} from './environment';
-import * as tf from './index';
-import {envSatisfiesConstraints, parseKarmaFlags, TestKernelBackend} from './jasmine_util';
+import {envSatisfiesConstraints, parseTestEnvFromKarmaFlags, TestEnv} from './jasmine_util';
 
 describe('jasmine_util.envSatisfiesConstraints', () => {
   it('ENV satisfies empty constraints', () => {
@@ -100,40 +99,50 @@ describe('jasmine_util.envSatisfiesConstraints', () => {
 });
 
 describe('jasmine_util.parseKarmaFlags', () => {
+  const registeredTestEnvs: TestEnv[] = [
+    {name: 'test-env', backendName: 'test-backend', isDataSync: true, flags: {}}
+  ];
+
   it('parse empty args', () => {
-    const res = parseKarmaFlags([]);
+    const res = parseTestEnvFromKarmaFlags([], registeredTestEnvs);
     expect(res).toBeNull();
   });
 
-  it('--backend test-backend --flags {"IS_NODE": true}', () => {
-    const backend = new TestKernelBackend();
-    tf.registerBackend('test-backend', () => backend);
-
-    const res = parseKarmaFlags(
-        ['--backend', 'test-backend', '--flags', '{"IS_NODE": true}']);
-    expect(res.name).toBe('test-backend');
+  it('--testEnv test-env --flags {"IS_NODE": true}', () => {
+    const res = parseTestEnvFromKarmaFlags(
+        ['--testEnv', 'test-env', '--flags', '{"IS_NODE": true}'],
+        registeredTestEnvs);
+    expect(res.name).toBe('test-env');
     expect(res.backendName).toBe('test-backend');
     expect(res.flags).toEqual({IS_NODE: true});
-
-    tf.removeBackend('test-backend');
   });
 
-  it('"--backend unknown" throws error', () => {
-    expect(() => parseKarmaFlags(['--backend', 'unknown'])).toThrowError();
+  it('"--testEnv unknown" throws error', () => {
+    expect(
+        () => parseTestEnvFromKarmaFlags(
+            ['--testEnv', 'unknown'], registeredTestEnvs))
+        .toThrowError();
   });
 
-  it('"--flags {}" throws error since --backend is missing', () => {
-    expect(() => parseKarmaFlags(['--flags', '{}'])).toThrowError();
+  it('"--flags {}" throws error since --testEnv is missing', () => {
+    expect(
+        () => parseTestEnvFromKarmaFlags(['--flags', '{}'], registeredTestEnvs))
+        .toThrowError();
   });
 
-  it('"--backend cpu --flags" throws error since features value is missing',
+  it('"--testEnv cpu --flags" throws error since features value is missing',
      () => {
-       expect(() => parseKarmaFlags(['--backend', 'cpu', '--flags']))
+       expect(
+           () => parseTestEnvFromKarmaFlags(
+               ['--testEnv', 'test-env', '--flags'], registeredTestEnvs))
            .toThrowError();
      });
 
   it('"--backend cpu --flags notJson" throws error', () => {
-    expect(() => parseKarmaFlags(['--backend', 'cpu', '--flags', 'notJson']))
+    expect(
+        () => parseTestEnvFromKarmaFlags(
+            ['--testEnv', 'test-env', '--flags', 'notJson'],
+            registeredTestEnvs))
         .toThrowError();
   });
 });

src/setup_test.ts

@@ -0,0 +1,34 @@
+/**
+ * @license
+ * Copyright 2019 Google LLC. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * =============================================================================
+ */
+
+/**
+ * This file is necessary so we register all test environments before we start
+ * executing tests.
+ */
+import './backends/cpu/backend_cpu_test_registry';
+import './backends/webgl/backend_webgl_test_registry';
+
+import {parseTestEnvFromKarmaFlags, setTestEnvs, TEST_ENVS} from './jasmine_util';
+
+// tslint:disable-next-line:no-any
+declare let __karma__: any;
+if (typeof __karma__ !== 'undefined') {
+  const testEnv = parseTestEnvFromKarmaFlags(__karma__.config.args, TEST_ENVS);
+  if (testEnv != null) {
+    setTestEnvs([testEnv]);
+  }
+}