643187 - Add support for generators to mochitests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Benjamin Stover (:stechz)]

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.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Part 1: Add generators as first-class members of mochitests

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.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Part 2: Timeout jsm

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.

Comment 3

[Benjamin Stover (:stechz)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Rob Campbell [:rc] (:robcee)]

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+!

Comment 7

[Benjamin Stover (:stechz)]

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?

Comment 8

[Rob Campbell [:rc] (:robcee)]

(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?

Comment 9

[Benjamin Stover (:stechz)]

> 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.

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Add support for generators to mochitests

Comment 11

[Benjamin Stover (:stechz)]

Attachment: Add support for generators to mochitests

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Add support for generators to mochitests

Comment 13

[Benjamin Stover (:stechz)]

Attachment: Add support for generators to mochitests

Comment 14

[Rob Campbell [:rc] (:robcee)]

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! :)

Comment 15

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/ce0f5ec7930f

Comment 16

[(no longer active)]

This only affects browser-chrome tests as far as I can see...

Comment 17

[Benjamin Stover (:stechz)]

Oh, you're right! We should have them for other mochitests as well.

Comment 18

[(no longer active)]

(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.

Comment 19

[Rob Campbell [:rc] (:robcee)]

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.

Comment 20

[Benjamin Stover (:stechz)]

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.

Comment 21

[Rob Campbell [:rc] (:robcee)]

Concur. :)

Comment 22

[(no longer active)]

(In reply to comment #21)
> Concur. :)

Righteous.

Comment 23

[Eric Shepherd [:sheppy]]

Can anyone update the documentation for this support? https://developer.mozilla.org/en/Mochitest

Comment 24

[Benjamin Stover (:stechz)]

Done! https://developer.mozilla.org/en/Mochitest

Comment 25

[Eric Shepherd [:sheppy]]

(In reply to comment #24)
> Done! https://developer.mozilla.org/en/Mochitest

My hero!

Comment 26

[Brendan Eich [:brendan]]

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

Comment 27

[Eric Shepherd [:sheppy]]

I've added documentation for isGenerator.

Comment 28

[Rob Campbell [:rc] (:robcee)]

woot!