Add Karma test for webworker

[WenheLI]

This PR adds test for webworker within Karma test.
Fell free to leave any feedback on the current solution!

---

This change is 

[WenheLI]

@dsmilkov - Hi there, could you help me review this PR?

[dsmilkov]

Nice progress. Took a first pass with a few comments.

Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

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

	...browserstackConfig,
	files: [
		'dist/setup_test.js',

remove tabs (use spaces), here and below

---

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

	files: [
		'dist/setup_test.js',
		{pattern: 'dist/**/worker_test.js'},

no need for pattern with **, just say "dist/worker_test.js"

---

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

		{pattern: 'dist/**/worker_test.js'},
		// Include tf-core into the static file
		{pattern: 'dist/**/tf-core.js', included: false}

no need for using a pattern with ** just saw dist/tf-core.js

---

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

  ],
  exclude: [
    'dist/test_node.js',

these files were not included by your files option, so no need to exclude them?

---

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

    'dist/test_async_backends.js'
  ],
  port: 12345

is there a reason to specify a port number explicitly?

---

scripts/test-ci.sh, line 39 at r1 (raw file):

  "run-browserstack --browsers=bs_chrome_mac --testEnv webgl2 --flags '{\"WEBGL_CPU_FORWARD\": false}'"

# Build tf-core for webworker using

Maybe say: "Build dist/tf-core.js which is used by the webworker test"

---

src/test_async_backends.ts, line 78 at r1 (raw file):

const runner = new jasmine();
// exclude worker test
runner.loadConfig({spec_files: ['dist/**/!(worker)_test.js'], random: false});

this regex worries me a bit. Can you provide the exclusion separately something like: spec_files: ['dist/**/*_test.js', '!dist/worker_test.js'] ?

---

src/test_node.ts, line 31 at r1 (raw file):

const runner = new jasmine();
// Exclude worker test in node ENV

same here. see comment above.

---

src/tests.ts, line 110 at r1 (raw file):

import './variable_test';
import './version_test';
import './worker_test';

we don't want this in here right? Can you change enumerate_tests.js to ignore it? See line 59 in enumerate_tests.js.

---

src/worker_test.ts, line 7 at r1 (raw file):

const workerTest = () => {
	//@ts-ignore
	importScripts('http://bs-local.com:12345/base/dist/tf-core.js');

can you omit the bs-local.com ? and just say /base/dist/tf-core.js. This way the test might work locally too.

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

src/test_async_backends.ts, line 78 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

this regex worries me a bit. Can you provide the exclusion separately something like: spec_files: ['dist/**/*_test.js', '!dist/worker_test.js'] ?

Actually, let's not ignore worker_test at all. See comment below.

---

src/test_node.ts, line 31 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

same here. see comment above.

Let's not ignore worker_test.js at all. See comment below.

---

src/tests.ts, line 110 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

we don't want this in here right? Can you change enumerate_tests.js to ignore it? See line 59 in enumerate_tests.js.

Actually let's keep worker_test here. See comment below.

---

src/worker_test.ts, line 21 at r1 (raw file):

};

describeWithFlags('computation in worker', ALL_ENVS, () => {

instead of passing ALL_ENVS, pass a custom HAS_WORKER constraint that is

const HAS_WORKER = {
  predicate: () => typeof(Worker) !== 'undefined' && typeof(Blob) !== 'undefined' && typeof(URL) !== 'undefined'
}

This way the it inside the describe will only run when there is a webworker present. This way you don't have to ignore worker_test.ts anywhere.

---

src/worker_test.ts, line 23 at r1 (raw file):

describeWithFlags('computation in worker', ALL_ENVS, () => {
  it('tensor in worker', (done) => {
		const worker = new Worker(fn2workerURL(workerTest));

Can you write this test in a way that runs in node.js too?

[WenheLI]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @WenheLI)

---

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

Previously, dsmilkov (Daniel Smilkov) wrote…

is there a reason to specify a port number explicitly?

Since we are using embedded worker here, we want the worker to have access to tf-core that requires an absolute address like http://localhost:12345/dist/tf-core.js. This is the reason for specifying the port number. I know this may not be the best practice but if we want to use the embedded worker, this seems to be the only solution for now.

---

src/worker_test.ts, line 7 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

can you omit the bs-local.com ? and just say /base/dist/tf-core.js. This way the test might work locally too.

Similar reason above that we are using embedded worker and ,in this way, we can not use relative address but absolute one. Btw, the bs-local.com does work locally as well, since karma does a favor of proxying here.

---

src/worker_test.ts, line 21 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

instead of passing ALL_ENVS, pass a custom HAS_WORKER constraint that is

const HAS_WORKER = {
  predicate: () => typeof(Worker) !== 'undefined' && typeof(Blob) !== 'undefined' && typeof(URL) !== 'undefined'
}

This way the it inside the describe will only run when there is a webworker present. This way you don't have to ignore worker_test.ts anywhere.

Thanks! That's a brilliant solution!

---

src/worker_test.ts, line 23 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Can you write this test in a way that runs in node.js too?

Similar reason above, due to the embedded worker, I need to use URL and Blob object which are not present in the node env.
The only solution comes into my mind is to request the test script as a separate file like what I have done before.

[WenheLI]

It just comes into my mind that separate worker test scripts might be a better approach. In this way, we can do the test under node env as well. Also, we might have a chance to reuse the existing unit test scripts. What is your opinion over this problem?

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @WenheLI)

[dsmilkov]

A few more follow up comments

Reviewed 3 of 7 files at r1, 5 of 5 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

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

Previously, WenheLI wrote…

Since we are using embedded worker here, we want the worker to have access to tf-core that requires an absolute address like http://localhost:12345/dist/tf-core.js. This is the reason for specifying the port number. I know this may not be the best practice but if we want to use the embedded worker, this seems to be the only solution for now.

Sounds good.

---

karma.conf.js, line 40 at r2 (raw file):

    'src/test_node.ts',
    'src/test_async_backends.ts',
    'src/worker_test.ts'

revert, no need to ignore worker_test

---

karma.conf.js, line 51 at r2 (raw file):

  files: ['dist/setup_test.js', {pattern: 'dist/**/*_test.js'}],
  exclude: [
    'dist/worker_test.js'

revert this change. Curious why you stopped ignoring test_node and test_async_backends?

---

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

  files: [
    'dist/setup_test.js',
    {pattern: 'dist/worker_test.js'},

you can just say 'path' like the line above (no need for pattern: 'path')

---

src/test_async_backends.ts, line 78 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Actually, let's not ignore worker_test at all. See comment below.

Same here. Revert this change. No need to ignore worker_test

---

src/test_node.ts, line 32 at r2 (raw file):

const runner = new jasmine();
// Exclude worker test in node ENV
runner.loadConfig({spec_files: ['dist/**/!(worker)_test.js'], random: false});

revert this change? we agreed to not ignore worker_test since the predicate function takes care of that right?

---

src/worker_test.ts, line 7 at r1 (raw file):

Previously, WenheLI wrote…

Similar reason above that we are using embedded worker and ,in this way, we can not use relative address but absolute one. Btw, the bs-local.com does work locally as well, since karma does a favor of proxying here.

Sounds good. Thanks!

---

src/worker_test.ts, line 23 at r1 (raw file):

Previously, WenheLI wrote…

Similar reason above, due to the embedded worker, I need to use URL and Blob object which are not present in the node env.
The only solution comes into my mind is to request the test script as a separate file like what I have done before.

Looks like you can use embedded workers in node. See https://nodejs.org/api/worker_threads.html#worker_threads_new_worker_filename_options (note where it says "If options.eval is true, this is a string containing JavaScript code rather than a path."). I think it should be possible.

Write the node.js test separately from the browser test but it should be in this file. You can use a different predicate that fires only when we are in node and we have the worker_threads package etc.

[WenheLI]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)

---

karma.conf.js, line 51 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

revert this change. Curious why you stopped ignoring test_node and test_async_backends?

Sorry about that. This change is made by mistake.

---

src/worker_test.ts, line 23 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Looks like you can use embedded workers in node. See https://nodejs.org/api/worker_threads.html#worker_threads_new_worker_filename_options (note where it says "If options.eval is true, this is a string containing JavaScript code rather than a path."). I think it should be possible.

Write the node.js test separately from the browser test but it should be in this file. You can use a different predicate that fires only when we are in node and we have the worker_threads package etc.

Done.

[WenheLI]

As the node script should not have the browserify evolved while the browser script indeed needs to have it. I currently put the two scripts into separate files. This might be the best practice for now.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)

[dsmilkov]

Sounds good. Just a few more comments but we are pretty close!

Reviewed 6 of 6 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

karma.conf.js, line 51 at r2 (raw file):

Previously, WenheLI wrote…

Sorry about that. This change is made by mistake.

This used to be test_node.js not test_node.ts. Can you revert? Tip: you can drag the brackets in the file matrix in reviewable to diff the file against any revision (in this case against base/master, which helps to see changes against master)

---

karma.conf.js, line 65 at r3 (raw file):

    'dist/setup_test.js',
    'dist/worker_test.js',
    // Include tf-core into the static file

Maybe rephrase this comment as: "Serve dist/tf-core.js as a static resource, but do not include in the test runner"

---

src/test_async_backends.ts, line 77 at r3 (raw file):

const runner = new jasmine();
// exclude worker test

This comment is outdated. Also once you rename to worker_node_test, you don't need to explicitly add it here. See comment below.

---

src/test_node.ts, line 31 at r3 (raw file):

const runner = new jasmine();
// Exclude worker test in node ENV

same here, comment is outdated, and remove the inclusion since worker_test_node should be renamed to worker_node_test

---

src/worker_test.ts, line 1 at r3 (raw file):

import {HAS_WORKER, describeWithFlags} from './jasmine_util';

add the License at the top (see other ts files)

---

src/worker_test.ts, line 10 at r3 (raw file):

  return URL.createObjectURL(blob);
};

add a comment that the following function is the source code of a web worker.

---

src/worker_test_node.ts, line 1 at r3 (raw file):

import {HAS_NODE_WORKER, describeWithFlags} from './jasmine_util';

All test files end with _test, so let's rename this to worker_node_test.ts to be consistent. Also because of that rename, you will have to make sure you ignore this from browserify tests, and not include is specifically for node tests.

---

src/worker_test_node.ts, line 1 at r3 (raw file):

import {HAS_NODE_WORKER, describeWithFlags} from './jasmine_util';

add the License at the top (see other ts files)

---

src/worker_test_node.ts, line 9 at r3 (raw file):

};

const workerTestNode = () => {

add a comment that this function is the body of a web worker.

[WenheLI]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)

---

karma.conf.js, line 51 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

This used to be test_node.js not test_node.ts. Can you revert? Tip: you can drag the brackets in the file matrix in reviewable to diff the file against any revision (in this case against base/master, which helps to see changes against master)

Done. Thanks for this tip!

[dsmilkov]

Just a couple of small comments. Nice work!

Reviewed 6 of 7 files at r4.
Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

scripts/test-ci.sh, line 42 at r4 (raw file):

yarn build-npm
# Run under webworker environment
yarn test-webworker --browsers=bs_safari_mac --testEnv webgl1 --flags '{"WEBGL_CPU_FORWARD": false, "WEBGL_SIZE_UPLOAD_UNIFORM": 0}'

remove --testEnv webgl1 --flag... I think this way it will run using both the cpu and webgl backends

---

src/worker_test.ts, line 3 at r4 (raw file):

/**
 * @license
 * Copyright 2017 Google Inc. All Rights Reserved.

Copyright 2019

---

src/worker_node_test.ts, line 3 at r4 (raw file):

/**
 * @license
 * Copyright 2017 Google Inc. All Rights Reserved.

Copyright 2019

[WenheLI]

Hi there, I think it is okay to be merged?

Reviewable status: 0 of 1 approvals obtained

[dsmilkov]

This is looking great. Thank you!