Note: There are a few cases of duplicates in user autocompletion which are being worked on.

DOM chrome worker tests do not really test anything

RESOLVED FIXED in mozilla9

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 559550 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #559550 - Flags: review?(ted.mielczarek)
Created attachment 559551 [details] [diff] [review]
Patch (v1)
Attachment #559550 - Attachment is obsolete: true
Attachment #559550 - Flags: review?(ted.mielczarek)
Attachment #559551 - Flags: review?(ted.mielczarek)
Created attachment 559552 [details] [diff] [review]
Patch (v1)

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

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/583b4c9b3d4e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.