Closed Bug 1000512 Opened 9 years ago Closed 9 years ago

If a test cleanup function returns a promise, wait for it to be resolved before running other tests


(Testing :: Mochitest, defect)

Not set


(Not tracked)



(Reporter: marco, Assigned: marco)




(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
I have an asynchronous cleanup function using OS.File and I need the cleanup function to finish before running other tests (see bug 981251 for a use case).
Attachment #8411344 - Flags: review?(jmaher)
Blocks: 981251
Attachment #8411344 - Attachment is obsolete: true
Attachment #8411344 - Flags: review?(jmaher)
I'm not sure how to do this with SimpleTest, as the comment at the beginning of testing/mochitest/tests/SimpleTest/SimpleTest.js says I should write cross-browser code here.
Flags: needinfo?(ted)
Flags: needinfo?(jmaher)
That ship has long sailed, so I wouldn't worry about it. I don't know that we have everything setup to use things like generators in Mochitest though, since it masquerades as web content, so you might have to figure out how to fix that via pref or whatever.
Flags: needinfo?(ted)
I think SimpleTest.js and related code has deviated from the original implementation.  The comments have not caught up.  That being said, there are minimal efforts to ensure our tests run cross browser- but don't let that stop you from adding something to SimpleTest to solve a problem.
Flags: needinfo?(jmaher)
Comment on attachment 8411344 [details] [diff] [review]

Review of attachment 8411344 [details] [diff] [review]:

I am asked for review here- is that still desirable?  It seems as though this was marked obsolete.  Please ask for review again if you would like a review!
Attached patch Patch (obsolete) — Splinter Review
I've sent the patch to try and it looks green:

There are only some known intermittent failures.
Attachment #8411918 - Flags: review?(jmaher)
Comment on attachment 8411918 [details] [diff] [review]

Review of attachment 8411918 [details] [diff] [review]:

::: testing/mochitest/browser-test.js
@@ +321,5 @@
>        let testScope = this.currentTest.scope;
>        while (testScope.__cleanupFunctions.length > 0) {
>          let func = testScope.__cleanupFunctions.shift();
>          try {
> +          yield func.apply(testScope);

how does yield work with exceptions?  Will it catch the same way as before?  Will program flow continue similar as before?

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +842,5 @@
>      SimpleTest._cleanupFunctions.push(aFunc);
>  };
> +
> +

nit: we don't need to add extra whitespace here, please remove these 2 new blank lines.
Attachment #8411918 - Flags: review?(jmaher) → review+
Attached patch PatchSplinter Review
(In reply to Joel Maher (:jmaher) from comment #6)
> how does yield work with exceptions?  Will it catch the same way as before? 
> Will program flow continue similar as before?

Yes, it will work as before.
Assignee: nobody → mar.castelluccio
Attachment #8411918 - Attachment is obsolete: true
Attachment #8412237 - Flags: review+
Keywords: checkin-needed
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1001681
I discussed with jmaher, and we're going to back this patch out.

SpecialPowers.wrap() is magical - really magical. It works well for small things, but it generally causes more intricate usages to fail in mysterious ways. In particular, doing SpecialPowers.Cu.import("resource://gre/modules/Task.jsm").Task is pure insanity - Task.jsm is a full-blown programming framework, and trying to wrap the entire thing in the magical transitive proxies is going to make things slow, error prone, and impossible to debug.

SpecialPowers.wrap() should be used sparingly, especially in the test harness that affects untold thousands of tests.

Try push for the backout:
We're using this "feature" in some tests, we probably need to translate the code to directly using promises.
Silly bug in the backout. Trying again:

(In reply to Marco Castelluccio [:marco] from comment #11)
> We're using this "feature" in some tests, we probably need to translate the
> code to directly using promises.

Ok - how many are there? Are you able to own this?
These are the tests that fail FWIW - all related to newtab:
Flags: needinfo?(mar.castelluccio)
We're using this in the toolkit/webapps/tests (they will fail intermittently otherwise).

I'm working on a patch.

In testing/mochitest/browser-test.js we were using Task.jsm even before this bug and AFAICS we're just importing it without SpecialPowers, so I think I don't need to modify that file too.
Flags: needinfo?(mar.castelluccio)
Depends on: 1028987
I've filed bug 1028987.
You need to log in before you can comment on or make changes to this bug.