On Thu, 14 Mar 2013 20:34:52 +0100, Arthur Barstow art.bars...@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-testsu...@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;querygt; 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
