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

Further modularize unit tests. #1665

Merged
merged 15 commits into from
Apr 11, 2019
Merged

Further modularize unit tests. #1665

merged 15 commits into from
Apr 11, 2019

Conversation

nsthorat
Copy link
Contributor

@nsthorat nsthorat commented Apr 5, 2019

This PR further modularizes backends.

  • Renames src/kernels to src/backends (code in these directories are more than just kernel code).
  • Convert TEST_ENVS to a registry. Test registries in backends add themselves to the set of backends to test against. The registry is completely deactivated if someone calls setTestEnvs or if the environment is setup via the command line.
  • Move any backend-specific tests into the respective backend directory.
  • Move WEBGL_ENVS and CPU_ENVS into the backend directories.

Two slightly funky tests are in engine_test that expect CPU / WebGL backends to be registered so that they can test moving data between real backends and calling ops. For now, I let these tests depend on existence of a backend in the engine_test.ts for now.

In a follow up CL I will add tsconfig.json rules to each of the backend directories to enforce modularization.

Verified the same number of tests run locally before and after (one test, 2D FFT, I made work in more environments to the number of tests actually increased).

This change is Reviewable

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.

Reviewed 127 of 127 files at r1.
Reviewable status: 0 of 1 approvals obtained

@nsthorat nsthorat changed the title WIP: Further modularize unit tests. Further modularize unit tests. Apr 5, 2019
@nsthorat nsthorat requested a review from dsmilkov April 5, 2019 22:04
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Can you quickly test that the project compiles if you add backends/webgl and backends/cpu to the exclude field in tsconfig.json?

Really nice cleanup!!! Wohoooo!

Reviewed 125 of 127 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)


src/engine_test.ts, line 300 at r2 (raw file):

 * things about backends being registered. These tests don't live in the
 * backend directories because it is testing engine rather than backend-specific
 * details but needs a real backend to exist. These test will fail if the

If the backend is not registered, this test won't fail right? Instead it will be ignored because it has a constraint of backends:'cpu' which will not be satisfied. Might be better to say that instead to avoid confusion.


src/engine_test.ts, line 372 at r2 (raw file):

});

describeWithFlags('Switching WebGL + CPU backends', {backends: 'webgl'}, () => {

can you have 2 backends in the constraint ? e.g. backends: ['webgl', 'cpu'], which means this test will be ignored unless both backends are in the registry. I just looked at jasmine_util and the current logic for backends is to match ANY, instead of ALL. If no tests needs ANY can we switch to ALL?


src/jasmine_util.ts, line 128 at r2 (raw file):

// Whether a call to setTestEnvs has been called so we turn off registration.
// This allows comamnd line overriding or programmatic overriding of the

typo: command

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 @dsmilkov)


src/engine_test.ts, line 300 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

If the backend is not registered, this test won't fail right? Instead it will be ignored because it has a constraint of backends:'cpu' which will not be satisfied. Might be better to say that instead to avoid confusion.

This test will only run when the CPU backend is active. This comment was supposed to cover the test below it as well, but I'll move these into two comments.


src/engine_test.ts, line 372 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

can you have 2 backends in the constraint ? e.g. backends: ['webgl', 'cpu'], which means this test will be ignored unless both backends are in the registry. I just looked at jasmine_util and the current logic for backends is to match ANY, instead of ALL. If no tests needs ANY can we switch to ALL?

So currently the backends means what backend is currently active not what backends are registered (which has different semantics). If we switch to what backends are registered, WebGL tests will run when we mean to run it just for CPU. I can add another special constraint here that says "backendsRegistered" and rename this one to "activeBackend" so that we capture these differences, what do you think?

Copy link
Contributor

@dsmilkov dsmilkov 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 @nsthorat)


src/engine_test.ts, line 300 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

This test will only run when the CPU backend is active. This comment was supposed to cover the test below it as well, but I'll move these into two comments.

Sgtm.


src/engine_test.ts, line 372 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

So currently the backends means what backend is currently active not what backends are registered (which has different semantics). If we switch to what backends are registered, WebGL tests will run when we mean to run it just for CPU. I can add another special constraint here that says "backendsRegistered" and rename this one to "activeBackend" so that we capture these differences, what do you think?

Ah got it. I set that up and already forgot how it works. I think it would be great to split it into "activeBackends" and "registeredBackends" to make that distinction.

Nikhil Thorat added 2 commits April 11, 2019 00:13
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


src/engine_test.ts, line 372 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Ah got it. I set that up and already forgot how it works. I think it would be great to split it into "activeBackends" and "registeredBackends" to make that distinction.

Done.

@nsthorat nsthorat merged commit 2675df1 into master Apr 11, 2019
@nsthorat nsthorat deleted the test-mod branch April 11, 2019 18:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants