Re: RfR: Web Workers Test Cases; deadline March 28

On Thu, 14 Mar 2013 20:34:52 +0100, Arthur Barstow <art.barstow@nokia.com>  
wrote:

> This is a WG-wide Request for Review [RfR] for the tests Microsoft and  
> Opera submitted for the Web Workers CR [CR]:
>
> <http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/>
> <http://w3c-test.org/webapps/Workers/tests/submissions/Opera/>
>
> Simon (Web Workers' `Test Facilitator`) proposed in [1] the tests are  
> sufficient to test the CR's exit criteria.
>
> If you have any comments, please send them to  
> public-webapps-testsuite@w3.org by March 28.

I've looked at all the files in  
http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/ , below  
are my comments.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/WorkerGlobalScope_ErrorEvent_message.htm

         assert_not_equals(err.message, undefined);
         assert_not_equals(err.message.indexOf(ErrorMessage), -1);

I think this assertion should be removed. The spec doesn't require any  
particular text in the message argument of onerror, so it doesn't make  
sense to check the value in a test, beyond checking that it is a string.  
I'd suggest the following assertion instead of the two above:

         assert_equals(typeof err.message, 'string');

(Same thing in Worker_ErrorEvent_message.htm.)

The WorkerGlobalScope_ErrorEvent tests are not testing ErrorEvent at all,  
but are testing WorkerGlobalScope#onerror and its arguments. I suggest  
renaming these tests and their description to be less confusing as to what  
they are testing.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/WorkerGlobalScope_EventTarget.htm

<title> WorkerGlobalScope implements EventTarget </title>
     var t = async_test("Test Description: WorkerGlobalScope implements  
EventTarget");

This doesn't affect what the test does, but in the spec the interface  
inherits from EventTarget, rather than implements it.

         assert_regexp_match(target, /\[object (.*?)Worker(.+?)\]/);

This should be instead:

         assert_equals(target, '[object WorkerGlobalScope]');

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/WorkerGlobalScope_removeEventListener.htm

This has trailing junk that should be removed.

This test also appears to be brittle as to whether it produces a result (I  
often get Not Run result). It appears to have to do with timing -- it sets  
a setTimeout that may or may not be longer than the timeout set in  
setup(). Other tests use the same pattern. I would recommend having a  
fixed number in setTimeout of, say, 100ms, and do this for all tests that  
contain `time * 2`. (Some tests do this already.)

             setTimeout(t.step_func(VerifyResult), time * 2);

change to

             setTimeout(t.step_func(VerifyResult), 100);

That appears to give more stable results for me. I also recommend changing  
the timeout in setup() in all tests to 2000ms since it might take some  
time to load the worker file, and I seem to get Not Run sometimes (when  
loading a test the first time) for some tests even if they're just waiting  
for a message event.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/WorkerLocation_hash_nonexist.htm

     var t = async_test("Test Description: WorkerLocation hash attribute  
returns an empty string when there is no &lt;query&gt; component in input  
URL.");

change to

     var t = async_test("Test Description: WorkerLocation hash attribute  
returns an empty string when there is no <query> component in input URL.");

(Similarly in other files, search for `&lt;`.)

     worker.postMessage(EvalScript);

EvalScript is not defined. I think the whole line can be removed. (Same  
error in some other files.)

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/WorkerNavigator_appName.htm

     var description = "WorkerNavigator appName: Returns the name of the  
browser: " + window.navigator.appName;

Remove window.navigator.appName from the description since it complicates  
comparing results of tests between browsers if the names of the tests  
differ between browsers. (Similarly for the other WorkerNavigator tests.)  
Test names should be fixed strings.

The spec now has a 'language' member for WorkerNavigator but there's no  
test for it.

There are some tests (e.g. the ErrorEvent tests) that are almost  
identical, connect to the same worker, get the same data, and only differ  
in what they check. This seems to be an inefficient way of doing things  
and makes the test suite take longer than necessary to run. I would  
recommend folding such tests together into one file and have separate  
test_async() objects for each thing that is to be tested.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/Worker_ErrorEvent_type.htm

All the Worker_ErrorEvent tests are wrong in that the ErrorEvent.js file  
catches the runtime error in onerror and then returns true, which means  
that the error is "handled", and no error event should be fired on the  
worker object (in the window context). The worker should not return true  
in onerror for these tests.

The "column" member of ErrorEvent is not tested.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/Worker_cross_origin_security_err.htm

This test is wrong, data: should not throw per spec.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/Worker_dispatchEvent_ErrorEvent.htm

This test is wrong, initErrorEvent was removed from the spec and replaced  
with event constructors. The test should check that initErrorEvent is  
*absent* and that the constructor works.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/postMessage_clone_port.htm

                 test(function(){ assert_equals(e.data, "ping"); }, "Data  
sent through remote port is received by the new cloned port");

This should create the test object up-front with async_test and do a  
step() here instead. Otherwise, if something fails before this code runs,  
there will be no test object at all, and that would be confusing when  
looking at the recorded results.

Alternatively, if it doesn't need to be a separate test, it can just do a  
step of the first test.

http://w3c-test.org/webapps/Workers/tests/submissions/Microsoft/postMessage_ports_readonly_array.htm

The bulk of the code should be in a step() since if e.g. `new   
MessageChannel()` throws, the harness doesn't catch it. This probably  
applies to most tests.

The postMessage tests appear to be testing MessageChannel rather than  
workers.

-- 
Simon Pieters
Opera Software

Received on Friday, 15 March 2013 10:18:11 UTC