734134 - test_simpleThread.html is() calls miss message parameter

Status: NEW

(Core :: DOM: Core & HTML, defect, P4)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1331207302.1331210117.7022.gz
WINNT 5.2 comm-aurora debug test mochitests-3/5 on 2012/03/08 03:48:22
{
7416 INFO TEST-PASS | /tests/dom/workers/test/test_simpleThread.html | undefined - [object Worker] should equal [object Worker]
[...]
}

Comment 1

[Alan Yeo [:alanyeojw]]

Attachment: add message parameters for is() calls

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 604135 [details] [diff] [review]
add message parameters for is() calls

That is the idea :-)
Yet using something (a little) "more specific" than "Correct xxx" would be better.
And try to make each message unique, so it's easier to match them with logs.

Comment 3

[Alan Yeo [:alanyeojw]]

Attachment: revise patch based on review

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 604448 [details] [diff] [review]
revise patch based on review

Review of attachment 604448 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I don't think your texts are that helpful.

::: dom/workers/test/test_simpleThread.html
@@ +23,5 @@
>  
>    var worker = new Worker("simpleThread_worker.js");
>  
>    worker.addEventListener("message",function(event) {
> +    is(event.target, worker, "Correct worker - \"inside message event listener\"");

Use "Expected event target for message".

@@ +28,5 @@
>      switch (event.data) {
>        case "no-op":
>          break;
>        case "started":
> +        is(gotErrors, true, "Correct errors - \"gotErrors == true\"");

Make that
is(gotErrors, 2, "Errors received before 'started' message");

@@ +42,5 @@
>          SimpleTest.finish();
>      }
>    }, false);
>  
>    var gotErrors = false;

Make that
var gotErrors = 0;

@@ +46,5 @@
>    var gotErrors = false;
>    worker.onerror = function(event) {
>      event.preventDefault();
> +    is(event.target, worker,
> +      "Correct worker - \"first error event triggered (uncaught exception)\"");

Use "Expected event target for 1st error".

@@ +47,5 @@
>    worker.onerror = function(event) {
>      event.preventDefault();
> +    is(event.target, worker,
> +      "Correct worker - \"first error event triggered (uncaught exception)\"");
> +    is(event.message, "uncaught exception: Bad message: asdf", "Correct error message");

Use "Expected 1st error".
Add
++gotErrors;

@@ +52,5 @@
>  
>      worker.onerror = function(otherEvent) {
>        otherEvent.preventDefault();
> +      is(otherEvent.target, worker,
> +        "Correct worker - \"second error event triggered (components not defined)\"");

Use "Expected event target for 2nd error".

@@ +53,5 @@
>      worker.onerror = function(otherEvent) {
>        otherEvent.preventDefault();
> +      is(otherEvent.target, worker,
> +        "Correct worker - \"second error event triggered (components not defined)\"");
> +      is(otherEvent.message, "Components is not defined", "Correct error message");

Use "Expected 2nd error".

@@ +58,1 @@
>        gotErrors = true;

Make that
++gotErrors;

@@ +58,4 @@
>        gotErrors = true;
>  
>        worker.onerror = function(oneMoreEvent) {
>          ok(false, "Worker had an error:" + oneMoreEvent.message);

Make that "Worker had an unexpected 3rd error:"

Comment 5

[Serge Gautherie (:sgautherie)]

And use a better patch comment than "fix bug-734134".

Comment 6

[Alan Yeo [:alanyeojw]]

Attachment: add message parameters to is() and ok() calls

add message parameters to is() and ok() calls
replace gotErrors with integer counter (was boolean flag previously)

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 606167 [details] [diff] [review]
add message parameters to is() and ok() calls

Review of attachment 606167 [details] [diff] [review]:
-----------------------------------------------------------------

This patch misses author and description. (Which your previous patches had.)

::: dom/workers/test/test_simpleThread.html
@@ +22,5 @@
>  <script class="testbody" type="text/javascript">
>  
>    var worker = new Worker("simpleThread_worker.js");
>  
>    worker.addEventListener("message",function(event) {

Add a space after ','.

@@ +28,5 @@
>      switch (event.data) {
>        case "no-op":
>          break;
>        case "started":
> +        is(gotErrors, 2, "Errors received before 'started' message");

Even "Number of errors ...".

@@ +37,5 @@
>          worker.postMessage("no-op");
>          SimpleTest.finish();
>          break;
>        default:
>          ok(false, "Unexpected message:" + event.data);

Add a space after ':'.

@@ +58,3 @@
>  
>        worker.onerror = function(oneMoreEvent) {
> +        ok(false, "Worker had an unexpected 3rd error:" + oneMoreEvent.message);

Add a space after ':'.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 606167 [details] [diff] [review]
add message parameters to is() and ok() calls

Ok, but in the future please don't change test functionality if it isn't broken (e.g. changing gotErrors from a boolean to an int counter). I don't care enough to say no, but it's an unnecessary change.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to ben turner [:bent] from comment #8)
> Ok, but in the future please don't change test functionality if it isn't
> broken (e.g. changing gotErrors from a boolean to an int counter). I don't
> care enough to say no, but it's an unnecessary change.

Let me explain why I asked for that: the counter will let us know whether there was 0 or 1 error _and_ that 2 errors were expected, which is more explicit than the boolean code is. (Nothing major, but a little improvement.)

Comment 10

[Alan Yeo [:alanyeojw]]

Attachment: re-submit patch based on feedback

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 611797 [details] [diff] [review]
re-submit patch based on feedback

Review of attachment 611797 [details] [diff] [review]:
-----------------------------------------------------------------

Use "Bug 734134. Add message parameter to is() calls, Enhance gotErrors variable to integer from boolean." as patch description.

::: dom/workers/test/test_simpleThread.html
@@ +28,5 @@
>      switch (event.data) {
>        case "no-op":
>          break;
>        case "started":
> +        is(gotErrors, 2, "Errors received before 'started' message");

Even "Number of errors received before 'started' message".

Comment 12

[Alan Yeo [:alanyeojw]]

Attachment: change commit message

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 612207 [details] [diff] [review]
change commit message

Review of attachment 612207 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/test_simpleThread.html
@@ +28,5 @@
>      switch (event.data) {
>        case "no-op":
>          break;
>        case "started":
> +        is(gotErrors, 2, "Even number of errors received before 'started' message");

"Even " was not meant to be part of this message...

Comment 14

[Alan Yeo [:alanyeojw]]

Attachment: extract Even from log statement

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 615102 [details] [diff] [review]
extract Even from log statement

No, "Even " was my bug comment, not to be in test code at all.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.