Closed Bug 643187 Opened 9 years ago Closed 9 years ago

Add support for generators to mochitests

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stechz, Assigned: stechz)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 5 obsolete files)

This is just a utility class we use in Fennec. It comes in handy for frame scripts and Cu.import scripts. It's a very light layer, but it adds a more straightforward method of starting the timer and can also check if a timer is pending.
This first part is useful in itself. This allows mochitests to yield, with a
function called "nextStep" to continue the execution of the test. Generators
have some advantages:

1. Errors can be checked after nextStep() is called. Normally, mochitests
cannot error check for callbacks.
2. More scalable tests. Many of our tests have a lot of necessary callbacks,
making it hard to track the order.

I think it's a matter of preference, but I know we have several tests already
that could benefit from this, and it's not disruptive for our other tests.

Mossop, can you review? hg blame suggested you know this code.
Attachment #520474 - Flags: review?(dtownsend)
Attached patch Part 2: Timeout jsm (obsolete) — Splinter Review
Since this comes from Fennec, I suppose I should see first if Mark thinks this
should be added to toolkit. Mark, if you don't think this is useful we should
probably stop using it in Fennec as well.
Attachment #520475 - Flags: review?(mark.finkle)
Assignee: nobody → ben
The more I think about this, the less I think the timeout code is really worth it. Still, I'd like to see the test generators land.
Might be worth having some of the JS folks look over the first patch; they may even be able to answer that question in the comments.
Comment on attachment 520474 [details] [diff] [review]
Part 1: Add generators as first-class members of mochitests

Probably better to get one of hte test peers to look at this. Not sure I like it myself, maybe it would be better to have a helper function do this rather than forcing test() to be the generator.

Might want to use waitForExplicitFinish rather than directly setting __done to false.
Attachment #520474 - Flags: review?(dtownsend) → review?(rcampbell)
Comment on attachment 520474 [details] [diff] [review]
Part 1: Add generators as first-class members of mochitests

+      var result = this.currentTest.scope.test();
+      if (result && "next" in result && "send" in result) {

you can test if (result.toString() == "[object Generator]"), but I wouldn't exactly call that "better" than what you have. I'd be curious to hear if there is a better way to test for this from the JS peeps as well.

I don't think there's anything wrong with setting self.__done = false directly as opposed to using waitForExplicitFinish(); I see the latter method as an external protocol, while __done is ok for use internally.

This could be nice to have! We have a bunch of browser tests that "roll their own" generator patterns and are somewhat confusing as a result.

r+!
Attachment #520474 - Flags: review?(rcampbell) → review+
Attachment #520475 - Attachment is obsolete: true
Attachment #520475 - Flags: review?(mark.finkle)
Changing the purpose of this bug to be about generators in tests.

Rob: someone in the JS channel had the excellent idea of comparing the toString generator function itself, as opposed to the return value. However, I got the general impression that we aren't meant to use objects that can be a function or a function that returns a generator.

We could call the entrance function generatorTest() perhaps? If both are defined, mochitest could register that as an error. What do you think?
Summary: Add Timeout component for easier use of nsITimer → Add support for generators to mochitests
(In reply to comment #7)
> Changing the purpose of this bug to be about generators in tests.
> 
> Rob: someone in the JS channel had the excellent idea of comparing the toString
> generator function itself, as opposed to the return value.

yeah, that'd work.

> However, I got the
> general impression that we aren't meant to use objects that can be a function
> or a function that returns a generator.

Why not? Too confusing?

> We could call the entrance function generatorTest() perhaps? If both are
> defined, mochitest could register that as an error. What do you think?

That makes sense, I think. It'd be pretty confusing if a test tried to define both and would hopefully not make it through a review. Wondering if that's being overly cautious?
> Why not? Too confusing?

I suppose. I don't find it confusing myself. Tons of JS libraries work with arguments that can be different types where it makes sense. I don't see this as any different, but not having a language-supported way of detecting a generator rains on my parade. :)

> That makes sense, I think. It'd be pretty confusing if a test tried to define
> both and would hopefully not make it through a review. Wondering if that's
> being overly cautious?

If we go with the generatorTest plan, I think an error is appropriate. I don't ever want to see a test defined with both. :)

So, I'll whip up a patch for generatorTest.
Attachment #520474 - Attachment is obsolete: true
Attachment #522487 - Flags: review?(rcampbell)
Attachment #522489 - Flags: review?(rcampbell)
Attachment #522487 - Attachment is obsolete: true
Attachment #522487 - Flags: review?(rcampbell)
Attachment #522509 - Flags: review?(rcampbell)
Attachment #522489 - Attachment is obsolete: true
Attachment #522489 - Flags: review?(rcampbell)
Attachment #522508 - Attachment is obsolete: true
Comment on attachment 522509 [details] [diff] [review]
Add support for generators to mochitests

looks good! I think all bugs should have at least 5 iterations of review request. Also, congrats on shipping for Android! :)
Attachment #522509 - Flags: review?(rcampbell) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/ce0f5ec7930f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
This only affects browser-chrome tests as far as I can see...
Oh, you're right! We should have them for other mochitests as well.
(In reply to comment #17)
> Oh, you're right! We should have them for other mochitests as well.

Hmm, I'm not sure how we should do that.  Mochitests do not have an entry point like browser chrome tests.
also, I'm not sure it's as necessary/useful there as it is for the browser chrome tests which have a bit of history of using generators there.

Do we have any examples of content mochitests that use generators? If not, I'd say leave it out until needed.
My use case was only for browser chrome tests, so I have no particular arguments for others. We can open a new bug if anyone ever finds a use for them in other types of tests.
Concur. :)
(In reply to comment #21)
> Concur. :)

Righteous.
Can anyone update the documentation for this support? https://developer.mozilla.org/en/Mochitest
BTW, thanks to dherman:

js> function f(){}
js> function g(){yield 42}
js> f.isGenerator()
false
js> g.isGenerator()
true

In for Firefox 5. Not sure whether MDC has docs yet or in progress -- Dave?

/be
I've added documentation for isGenerator.
Component: General → BrowserTest
Flags: in-testsuite-
Product: Core → Testing
QA Contact: general → browsertest
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.