Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

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

Merged
merged 8 commits into from Jun 13, 2019

Conversation

nsthorat
Copy link
Contributor

@nsthorat nsthorat commented Jun 13, 2019

Previously the test registries would happen as a function of imports. This doesn't ensure that unconstrained tests run after 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.


This change is Reviewable

Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but i think there are some opportunities to make things clearer/more explicit.

Also update https://github.com/tensorflow/tfjs-core/blob/master/DEVELOPMENT.md

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @nsthorat, and @tafsiri)


karma.conf.js, line 64 at r1 (raw file):

  const args = [];
  if (config.testEnv) {
    args.push('--testEnv', config.testEnv);

What happens if no --testEnv is passed? (document that here?)


scripts/enumerate-tests.js, line 44 at r1 (raw file):

const path = require('path');
const argv = require('yargs').argv;

Optional: Add some docs to describe the purpose of this file.


src/jasmine_util.ts, line 64 at r1 (raw file):

}

export function parseKarmaFlags(

this should be renamed to indicate that is is specifically trying to parse TestEnv related info from flags.


src/setup_test.ts, line 32 at r1 (raw file):

  const testEnv = parseKarmaFlags(__karma__.config.args, TEST_ENVS);
  if (testEnv != null) {
    setTestEnvs([testEnv]);

Not related to this line, but can we add a beforeAll check to the test runner infra to confirm that an environment has been set has been set before any test runs? (to guard against the files entry in config being modified.

Copy link
Contributor Author

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @tafsiri)


karma.conf.js, line 64 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

What happens if no --testEnv is passed? (document that here?)

Done.


scripts/enumerate-tests.js, line 44 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Optional: Add some docs to describe the purpose of this file.

Done.


src/jasmine_util.ts, line 64 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

this should be renamed to indicate that is is specifically trying to parse TestEnv related info from flags.

Done.


src/setup_test.ts, line 32 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Not related to this line, but can we add a beforeAll check to the test runner infra to confirm that an environment has been set has been set before any test runs? (to guard against the files entry in config being modified.

Done. I did this at the beginning of describeWithFlags before we iterate through the test envs.

@nsthorat nsthorat merged commit 92df253 into master Jun 13, 2019
@nsthorat nsthorat deleted the tests branch June 13, 2019 20:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants