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

Add Karma test for webworker #1841

Merged
merged 13 commits into from
Jul 25, 2019
Merged

Conversation

WenheLI
Copy link
Contributor

@WenheLI WenheLI commented Jul 16, 2019

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


This change is Reviewable

@WenheLI
Copy link
Contributor Author

WenheLI commented Jul 17, 2019

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

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.

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.

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: 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?

Copy link
Contributor Author

@WenheLI WenheLI 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: 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.

Copy link
Contributor Author

@WenheLI WenheLI left a comment

Choose a reason for hiding this comment

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

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)

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.

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.

Copy link
Contributor Author

@WenheLI WenheLI 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: 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.

Copy link
Contributor Author

@WenheLI WenheLI left a comment

Choose a reason for hiding this comment

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

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)

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.

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.

Copy link
Contributor Author

@WenheLI WenheLI 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: 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!

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.

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

Copy link
Contributor Author

@WenheLI WenheLI left a comment

Choose a reason for hiding this comment

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

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

Reviewable status: 0 of 1 approvals obtained

@dsmilkov
Copy link
Contributor

This is looking great. Thank you!

@dsmilkov dsmilkov merged commit ab6b88d into tensorflow:master Jul 25, 2019
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