Last Comment Bug 685995 - DOM chrome worker tests do not really test anything
: DOM chrome worker tests do not really test anything
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: 668728
  Show dependency treegraph
 
Reported: 2011-09-09 13:25 PDT by :Ehsan Akhgari
Modified: 2011-09-23 04:40 PDT (History)
7 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (27.01 KB, patch)
2011-09-09 13:26 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1) (27.01 KB, patch)
2011-09-09 13:27 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1) (27.54 KB, patch)
2011-09-09 13:29 PDT, :Ehsan Akhgari
ted: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2011-09-09 13:25:29 PDT
I found out about this bug when I was working on bug 668728.

The waitForExplicitFinish/finish semantics are not counter based, which means that each test is supposed to call each of these functions once, and only once.  This is not enforced by the framework currently (I'm also going to file a bug about that), but anyways.  What happens is that some of these tests issue several finish calls, and those will cause future tests to effectively be skipped.

I have a patch to make these tests call these functions only once, without any logic change to them.
Comment 1 :Ehsan Akhgari 2011-09-09 13:26:31 PDT
Created attachment 559550 [details] [diff] [review]
Patch (v1)
Comment 2 :Ehsan Akhgari 2011-09-09 13:27:52 PDT
Created attachment 559551 [details] [diff] [review]
Patch (v1)
Comment 3 :Ehsan Akhgari 2011-09-09 13:29:04 PDT
Created attachment 559552 [details] [diff] [review]
Patch (v1)

This time, actually including the file!
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-09-22 13:06:03 PDT
Comment on attachment 559552 [details] [diff] [review]
Patch (v1)

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

This makes me wonder if we shouldn't just rework the Mochitest harness APIs to work more like the xpcshell harness APIs. If we made waitForExplicitFinish() and finish() mutliply-callable, the simple case would still work, but usage like this would be possible.

::: dom/workers/test/dom_worker_helper.js
@@ +6,5 @@
> +var gRemainingTests = 0;
> +
> +function waitForWorkerFinish() {
> +  if (++gRemainingTests == 1) {
> +    SimpleTest.waitForExplicitFinish();

I'm not really a fan of using prefix/postfix operators inside a boolean expression, I'd prefer if you stuck the increment in its own statement. (Plus then you can just make the test if (gRemainingTests == 0) {}.)

@@ +13,5 @@
> +
> +function finish() {
> +  if (--gRemainingTests == 0) {
> +    SimpleTest.finish();
> +  }

Are these guaranteed not to race? That is, if you set up a test and it winds up finishing quickly, and calling finish() before you call waitForWorkerFinish() for the next test, that seems like it'd be a problem.

::: dom/workers/test/test_file.xul
@@ +72,4 @@
>      };
>  
>      worker.postMessage(file);
> +    waitForWorkerFinish();

Feels like it'd be better to move the waitForWorkerFinish() to the top of the test function.
Comment 5 :Ehsan Akhgari 2011-09-22 19:38:04 PDT
(In reply to Ted Mielczarek [:ted, :luser] from comment #4)
> @@ +13,5 @@
> > +
> > +function finish() {
> > +  if (--gRemainingTests == 0) {
> > +    SimpleTest.finish();
> > +  }
> 
> Are these guaranteed not to race? That is, if you set up a test and it winds
> up finishing quickly, and calling finish() before you call
> waitForWorkerFinish() for the next test, that seems like it'd be a problem.

The way that the tests work right now, all of the waitForWorkerFinish calls are made in sequence without hitting the event loop, so racing is not an issue.

Pushed with the rest of the comments addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/583b4c9b3d4e
Comment 6 Ed Morley [:emorley] 2011-09-23 04:40:45 PDT
https://hg.mozilla.org/mozilla-central/rev/583b4c9b3d4e

Note You need to log in before you can comment on or make changes to this bug.