756607 - Rewrite device WebAPI Marionette tests

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

With bug 754178 and bug 754216 in place, we can start writing simpler Marionette tests against some device WebAPIs entirely in JS.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Rewrite battery tests

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 626136 [details] [diff] [review]
Part 1 (v1): Rewrite battery tests

Review of attachment 626136 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

For code like so:

> runEmulatorCmd("power status not-charging", function(result) {
>   is(result[0], "OK");
>  
>   ok(EventTracker.seen.chargingchange, "Seen a 'chargingchange' event");
>   verifyAPI(discharging);
> });

it seems we're assuming that the change to the battery state will propagate into the gecko event handler by the time the Marionette emulator callback is fired...will that always be the case?  I have no idea.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> it seems we're assuming that the change to the battery state will propagate
> into the gecko event handler by the time the Marionette emulator callback is
> fired...will that always be the case?  I have no idea.

From my observation I thought this was the case. But you're right, we should probably not rely on this. I'll look into writing a helper to synchronize on both events/callbacks so that we don't depend on their order.

Comment 4

[Jonathan Griffin (:jgriffin)]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Jonathan Griffin (:jgriffin) from comment #2)
> > it seems we're assuming that the change to the battery state will propagate
> > into the gecko event handler by the time the Marionette emulator callback is
> > fired...will that always be the case?  I have no idea.
> 
> From my observation I thought this was the case. But you're right, we should
> probably not rely on this. I'll look into writing a helper to synchronize on
> both events/callbacks so that we don't depend on their order.

Maybe it's a safe bet.  The Marionette event has to go out to the Python testrunner via telnet, and then back into gecko over the remote debugger socket, so perhaps it will always be slow enough that the gecko event handler will get notified first.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not following closely but relying on timing is not a good idea.  Can we investigate a way to strongly order what needs to be ordered here?

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> I'm not following closely but relying on timing is not a good idea.  Can we
> investigate a way to strongly order what needs to be ordered here?

I'm pretty sure we can't guarantee an order of events happening between multiple processes that are talking over telnet sockets and emulated devices etc. I think the only thing we can expect here is to correlate two events.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's chat.  IRC ping outstanding.

Comment 8

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Let's chat.  IRC ping outstanding.

Was there a conclusion to this that can be rolled up into the bug?

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Rewrite battery tests

cjones raised a good point in our IRC discussion: when we're talking to the emulator to manipulate state, we really only care about the resulting Gecko event. If the emulator response code indicates a failure, we're hosed anyway and the whole run is pretty much invalid. Marionette already checks emulator return codes, so where appropriate, the test flow should be controlled by Gecko events. Filed bug 758448 to make runEmulatorCmd callbacks optional.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 627034 [details] [diff] [review]
Part 1 (v2): Rewrite battery tests

Re: not wanting to go off the emulator for order, wouldn't that apply to stuff like full() or discharging() too? 

I suspect you're using the emulator because we don't expect an event to fire, but I think that leaves the code open to a race where it calls verifyAPI before an extraneous event fires and doesn't realize it fired. 

Instead, perhaps it should wait to see if an event fires within a fixed timeout, and pass when it does not.

Also, 

+  runEmulatorCmd("power capacity " + newCapacity);
+
+  battery.onlevelchange = function onlevelchange(event) {
+    battery.onlevelchange = unexpectedEvent;
+    verifyAPI(charging);
+  };

I realize the chance that the event fires before the assignment to onlevelchange can happen is slim, but would the safest bet be to assign the handler before running the emulator command?

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Geo Mealer [:geo] from comment #10)
> Comment on attachment 627034 [details] [diff] [review]
> Part 1 (v2): Rewrite battery tests
> 
> Re: not wanting to go off the emulator for order, wouldn't that apply to
> stuff like full() or discharging() too? 
> 
> I suspect you're using the emulator because we don't expect an event to
> fire,

Correct. In fact, we fail if we see events firing, thanks to unexpectedEvent.

> but I think that leaves the code open to a race where it calls
> verifyAPI before an extraneous event fires and doesn't realize it fired.
> 
> Instead, perhaps it should wait to see if an event fires within a fixed
> timeout, and pass when it does not.

Proving the non-existence of an event is always tricky. In the cases you're worried about, an event would have to race at least two runEmulatorCmd() calls. I find that pretty unlikely. But ok, if you would feel better, we can play the waiting game like you suggest. I just find it hard to come up with a number that works on developer machines as well as CI.

> +  runEmulatorCmd("power capacity " + newCapacity);
> +
> +  battery.onlevelchange = function onlevelchange(event) {
> +    battery.onlevelchange = unexpectedEvent;
> +    verifyAPI(charging);
> +  };
> 
> I realize the chance that the event fires before the assignment to
> onlevelchange can happen is slim, but would the safest bet be to assign the
> handler before running the emulator command?

It's impossible. JS is single threaded, nothing can run between those two lines.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 627034 [details] [diff] [review]
Part 1 (v2): Rewrite battery tests

Looks good.  I'm not sure what to do to verify that events don't occur either; waiting a couple of seconds is probably generally safe, but there's never any guarantee...

Comment 13

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Thanks for the clarification re: threading and why you're unconcerned. I'll have to reconcile my understanding of async functions with that (mostly when control of the thread can pass). It'll be a fun exercise. :)

Re: the waiting, it's true that the best we could do is "no event within N time." Sounds like you don't think it's terribly valuable and is a lot of hassle and I'm fine going with your instinct on it. 

OTOH, if we had a general pattern or helper for that, there are a lot of APIs that throw events that could use it. We can always revisit that later.

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (WIP 1): Rewrite telephony tests

Started converting the telephony tests. A couple of empty tests still, will tackle those asap.

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Rewrite SMS tests

Also got a WIP for SMS tests.

Comment 16

[Jonathan Griffin (:jgriffin)]

Comment on attachment 627082 [details] [diff] [review]
Part 2 (WIP 1): Rewrite telephony tests

Review of attachment 627082 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I like!

::: dom/telephony/test/marionette/test_outgoing_answer_hangup.js
@@ +33,5 @@
> +  ok(outgoing);
> +  is(outgoing.number, number);
> +  is(outgoing.state, "dialing");
> +
> +  //is(ouygoing, telephony.active); // bug 757587

ouygoing -> outgoing

Comment 17

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Rewrite telephony tests

Still a bunch of stuff to do on this front, but I think we should defer these to follow-ups. Filed bug 759521, 757587, 759533.

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 627084 [details] [diff] [review]
Part 3 (v1): Rewrite SMS tests

I'm going to call this ready for now. Filed bug 759529 for testing outgoing SMS separately.

Comment 19

[Jonathan Griffin (:jgriffin)]

Comment on attachment 628118 [details] [diff] [review]
Part 2 (v1): Rewrite telephony tests

Review of attachment 628118 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 20

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/c1ad9e9f6632
https://hg.mozilla.org/mozilla-central/rev/013b22ee36b7
https://hg.mozilla.org/mozilla-central/rev/1c1318dbdf55