Run ts-node tests with --transpile-only to increase speed

[mattsoulanille]

This change nearly halves the CI run time and also significantly reduces local test time. It's safe to run the tests in transpile-only mode since the source that is being tested has already been typechecked when it was compiled. The change may make it harder to find type errors in the test code, which is definitely a trade off to consider before merging.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

---

This change is 

[lina128]

Cutting by half is a great improvement, dev velocity is definitely worth the tradeoff imho. Thank you Matt! Wait for Yannick and Ping's comments too.

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @tafsiri)

[pyu10055]

Thank you!

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @mattsoulanille, @pyu10055, and @tafsiri)

---

tfjs-data/package.json, line 61 at r1 (raw file):

    "publish-local": "rimraf dist/ && yarn build-npm && yalc push",
    "publish-npm": "npm publish",
    "test": "yarn && yarn build-deps && ts-node --transpile-only --project tsconfig.test.json src/test_node.ts",

here since it is building the deps, it would be good to add yarn build to ensure the package is also built. This should apply to other packages as well.

[pyu10055]

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

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

here since it is building the deps, it would be good to add yarn build to ensure the package is also built. This should apply to other packages as well.

The point is we want to be consistent across packages on what are the steps for running the test locally? yarn build && yarn test or something else.

[lina128]

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

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

The point is we want to be consistent across packages on what are the steps for running the test locally? yarn build && yarn test or something else.

We leave the build step out to make local testing as light as possible. But if leaving build step out will make it harder to find type error in the package to test, then we should add building step.

[mattsoulanille]

Good point! I've added yarn build to all the tests that run with ts-node.

[tafsiri]

Reviewed 5 of 16 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @mattsoulanille, @pyu10055, and @tafsiri)

---

package.json, line 29 at r3 (raw file):

    "publish-npm": "ts-node -s ./scripts/publish-npm.ts",
    "release-notes": "ts-node -s ./scripts/release_notes/release_notes.ts",
    "test-release-notes": "ts-node --transpile-only -s ./scripts/release_notes/run_tests.ts",

same comment as elsewhere about this being the only place this might get typed checked as it is not part of src.

---

tfjs/integration_tests/validate_converter.sh, line 146 at r1 (raw file):

  echo "Starting validation karma tests in Node.js..."
  yarn ts-node --transpile-only run_node_tests.ts \

is run_node_tests.ts built by tsc/yarn build. If not then i think this is the only opportunity for type checking these files.

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

We leave the build step out to make local testing as light as possible. But if leaving build step out will make it harder to find type error in the package to test, then we should add building step.

If we add the build step here would we not lose the speed benefits of transpile-only? (i.e. is tsc + ts-node --transpile-only ... equivalent to ts-node ...) . Are these local builds faster than they were before now that you have added yarn build? Could you add some timings to the PR description?

One approach we could do if we only use transpile-only in test-ci, since we know the package would have already been built, and locally leave things as is (somewhat dependent on the answer to the question above?

---

tfjs-node/integration/typescript/package.json, line 8 at r1 (raw file):

  "scripts": {
    "prep": "yalc link @tensorflow/tfjs-node && cd node_modules/@tensorflow/tfjs-node && yarn clean-deps && yarn",
    "test": "ts-node --transpile-only -P tsconfig.test.json src/test.ts && tsc && node dist/test.js"

Is this whole folder obsolete? It doesn't seem to have srcs anymore. I can't see how yarn test would work? (not exactly related to this PR)

[pyu10055]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @tafsiri)

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

If we add the build step here would we not lose the speed benefits of transpile-only? (i.e. is tsc + ts-node --transpile-only ... equivalent to ts-node ...) . Are these local builds faster than they were before now that you have added yarn build? Could you add some timings to the PR description?

One approach we could do if we only use transpile-only in test-ci, since we know the package would have already been built, and locally leave things as is (somewhat dependent on the answer to the question above?

agree, we should definitely add --transpile-only to ci targets. for local test, the 'test' target is currently already including everything, it is not consistent to omit build step.
If we want to make test lightweight, we should another target, 'test-dev' does nothing but ts-node --transplie-only

[lina128]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @mattsoulanille, @pyu10055, and @tafsiri)

---

package.json, line 29 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

same comment as elsewhere about this being the only place this might get typed checked as it is not part of src.

Nice catch. On another hand, do we care whether these tests are typechecked or not?

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

agree, we should definitely add --transpile-only to ci targets. for local test, the 'test' target is currently already including everything, it is not consistent to omit build step.
If we want to make test lightweight, we should another target, 'test-dev' does nothing but ts-node --transplie-only

Ah, great point!!! Agree with both Yannick and Ping, use transpile-only for ci and test-dev. Please add test-dev to all packages for consistency.

[mattsoulanille]

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

---

package.json, line 29 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

Nice catch. On another hand, do we care whether these tests are typechecked or not?

It takes 1.5 seconds with typechecking, so I've added it back in. I should mention that there are a few other cases where the test launchers won't get typechecked if we go the transpile-only route. Anything with a run_tests.ts file at the root won't have that file typechecked:

./scripts/release_notes/run_tests.ts
./tfjs/run_tools_tests.ts
./tfjs/integration_tests/run_node_tests.ts
./tfjs-converter/run_tests.ts
./tfjs-backend-cpu/run_tests.ts

It might be better to compile run_tests.ts against the outputs of yarn build with tsc and run it with node. That way, when it gets typechecked, we don't re-typecheck the entire project. I can look into this.

---

tfjs/integration_tests/validate_converter.sh, line 146 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

is run_node_tests.ts built by tsc/yarn build. If not then i think this is the only opportunity for type checking these files.

I don't think it is built by yarn build.See my other comment about compiling tests with tsc as a possible solution.

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Ah, great point!!! Agree with both Yannick and Ping, use transpile-only for ci and test-dev. Please add test-dev to all packages for consistency.

@tafsiri Running yarn build with transpile-only after building the package in a previous step is actually a bit faster due to incremental typescript compilation, which ts-node can't take advantage of. I get 0m52.220s when running yarn build and testing with transpile-only, and I get 1m7.020s without yarn build and without transpile-only.

It would definitely still be better not to run yarn build again in CI like you mentioned, since yarn build also runs rollup, which I haven't figured out how to make incremental yet (although Bazel could help with that). I'll add a test-ci rule to each package as mentioned above.

---

tfjs-node/integration/typescript/package.json, line 8 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Is this whole folder obsolete? It doesn't seem to have srcs anymore. I can't see how yarn test would work? (not exactly related to this PR)

I'm pretty sure it is. I was doing a find-replace to find these instances, and I didn't check this one's files. I'll remove it.

[pyu10055]

Reviewed 13 of 16 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @pyu10055 and @tafsiri)

[lina128]

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

---

package.json, line 29 at r3 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

It takes 1.5 seconds with typechecking, so I've added it back in. I should mention that there are a few other cases where the test launchers won't get typechecked if we go the transpile-only route. Anything with a run_tests.ts file at the root won't have that file typechecked:

./scripts/release_notes/run_tests.ts
./tfjs/run_tools_tests.ts
./tfjs/integration_tests/run_node_tests.ts
./tfjs-converter/run_tests.ts
./tfjs-backend-cpu/run_tests.ts

It might be better to compile run_tests.ts against the outputs of yarn build with tsc and run it with node. That way, when it gets typechecked, we don't re-typecheck the entire project. I can look into this.

This is a great point. Should we just move run_tests to src/

[mattsoulanille]

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

---

package.json, line 29 at r3 (raw file):

Previously, lina128 (Na Li) wrote…

This is a great point. Should we just move run_tests to src/

I've been working on that, but I'm running into problems with running the build outputs with node. Everything gets compiled into .js files that use import syntax, which node doesn't really support, instead of require syntax.

[mattsoulanille]

This is a bit stale, but I think it should still be merged. I've solved the typechecking issue by first running tsc to check types and then running ts-node in transpile-only mode. There were a few cases where there wasn't an easy way to compile with tsc (some .ts files were in the /scripts directory and I didn't think it would be good to move them to /src), so I'm running those with typechecking enabled in ts-node. The most expensive .ts files were the test files, and they were all moved to /src of their respective packages.

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

---

package.json, line 29 at r3 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

I've been working on that, but I'm running into problems with running the build outputs with node. Everything gets compiled into .js files that use import syntax, which node doesn't really support, instead of require syntax.

I was initially trying to run the compiled outputs directly with node, which didn't work since node doesn't support esm yet. I was also trying to create a custom bundle for tests using webpack, but it's much easier to just run ts-node on the typescript files. I've moved the test files to /src and made sure to only run ts-node in transpile mode if the sources it's running against have recently been compiled (each test that runs ts-node in transpile mode now runs tsc beforehand).

---

tfjs-data/package.json, line 61 at r1 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

@tafsiri Running yarn build with transpile-only after building the package in a previous step is actually a bit faster due to incremental typescript compilation, which ts-node can't take advantage of. I get 0m52.220s when running yarn build and testing with transpile-only, and I get 1m7.020s without yarn build and without transpile-only.

It would definitely still be better not to run yarn build again in CI like you mentioned, since yarn build also runs rollup, which I haven't figured out how to make incremental yet (although Bazel could help with that). I'll add a test-ci rule to each package as mentioned above.

I've added test-dev to all packages.

[tafsiri]

I think i found one are where the test scripts are never typechecked, PTAL.

Generally I am of the opinion if we have a ts file it should be run through the typechecker at some point in time.

Reviewed 2 of 16 files at r1, 21 of 21 files at r4.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @mattsoulanille, @pyu10055, and @tafsiri)

---

tfjs/integration_tests/validate_converter.sh, line 146 at r1 (raw file):

Previously, mattsoulanille (Matthew Elliott Soulanille) wrote…

I don't think it is built by yarn build.See my other comment about compiling tests with tsc as a possible solution.

We should then add a yarn build step somewhere in the test scripts for these intrgration tests, else we never typecheck these files. (Or I would not use transpile-only here).

---

tfjs-core/package.json, line 82 at r4 (raw file):

    "test-async-backends": "rimraf dist/ && yarn build && ts-node --transpile-only -P tsconfig.test.json dist/test_async_backends.js",
    "test-async-backends-ci": "ts-node --transpile-only -P tsconfig.test.json dist/test_async_backends.js",
    "test-snippets": "yarn build && yarn build-cpu-backend && ts-node --transpile-only -P tsconfig.test.json ./scripts/test_snippets/test_snippets.ts"

this test-snippets is different from the others (they just use ts-node).

[mattsoulanille]

Thanks for catching those cases! I completely agree that all .ts files should be typechecked at some point.

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

---

tfjs/integration_tests/validate_converter.sh, line 146 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

We should then add a yarn build step somewhere in the test scripts for these intrgration tests, else we never typecheck these files. (Or I would not use transpile-only here).

Nice catch! I missed this one when removing --transpile-only from targets that don't get built with tsc.

---

tfjs-core/package.json, line 82 at r4 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

this test-snippets is different from the others (they just use ts-node).

Good catch! Switched it back to not using --transpile-only.