Closed Bug 685995 Opened 9 years ago Closed 9 years ago

DOM chrome worker tests do not really test anything

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #559550 - Flags: review?(ted.mielczarek)
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #559550 - Attachment is obsolete: true
Attachment #559550 - Flags: review?(ted.mielczarek)
Attachment #559551 - Flags: review?(ted.mielczarek)
Attached patch Patch (v1)Splinter Review
This time, actually including the file!
Attachment #559551 - Attachment is obsolete: true
Attachment #559551 - Flags: review?(ted.mielczarek)
Attachment #559552 - Flags: review?(ted.mielczarek)
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #559552 - Flags: review?(ted.mielczarek) → review+
(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
Flags: in-testsuite+
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/583b4c9b3d4e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.