900969 - Failing deviceorientation test_bug615597.html for b2g mochitest

Status: REOPENED

(Core :: DOM: Device Interfaces, defect, P5)

Description

[Martijn Wargers (dead)]

Attachment: 615597.diff

The attached patch makes this test pass against my local b2g emulator mochitest run, while it would previously fail with these errors:
0 INFO SimpleTest START
1 INFO TEST-START | /tests/content/events/test/test_bug615597.html
2 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | Should have seen DeviceOrientationEvent!
3 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | undefined
4 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | undefined
5 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | undefined
6 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | undefined
7 INFO TEST-END | /tests/content/events/test/test_bug615597.html | finished in 1472ms
8 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug615597.html | undefined - got 0, expected 1.5
9 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug615597.html | undefined - got -90, expected 2.25
10 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug615597.html | undefined - got 0, expected 3.667
11 INFO TEST-PASS | /tests/content/events/test/test_bug615597.html | undefined
12 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug615597.html | [SimpleTest.finish()] this test already called finish!
13 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_bug615597.html | called finish() multiple times

Comment 1

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=37fe9b265ffa

Comment 2

[Alon Zakai (:azakai)]

Comment on attachment 785002 [details] [diff] [review]
615597.diff

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

I haven't worked on this for years, so I am probably not the right person to review it I am afraid.

One comment though, adding a setTimeout of 0 was frowned upon last I heard, because it can cause intermittent failures. But again, my information could be out of date.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

setTimeout is no worse than executeSoon.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785002 [details] [diff] [review]
615597.diff

But this shouldn't fail without timeout.

Comment 5

[Martijn Wargers (dead)]

Ok, but isn't there a potential problem here?
waitForExplicitFinish called after the event being invoked, which means that Simpletest.finish can be called before Simpletest.waitForExplicitFinish?

There are also these tests, which look very similar to this test (they were based on this test):
http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug667919-1.html?force=1
http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug667919-2.html?force=1

test_bug667919-1.html and test_bug667919-2.html are actually the same, I think that one of those 2 tests was meant to use devicemotion instead.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785002 [details] [diff] [review]
615597.diff

Oh, I missed that. Wouldn't this pass if you just
remove SimpleTest.waitForExplicitFinish() and SimpleTest.finish();

Comment 7

[Martijn Wargers (dead)]

Then wouldn't there be a risk that the deviceorientation event is received after the load event has been fired (which means in mochitest terms that the test is finished, when to waitForExplicitFinish)?

Comment 8

[Martijn Wargers (dead)]

Btw, I'm not sure that the patch that I attached (with or without that setTimeout call) would fix the failures in b2g, I seem to still get that error sometimes in b2g.

But probably still better to fix this test and those other tests I mentioned in comment 4 and change test_bug667919-2.html to use devicemotion.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #7)
> Then wouldn't there be a risk that the deviceorientation event is received
> after the load event has been fired (which means in mochitest terms that the
> test is finished, when to waitForExplicitFinish)?

But you dispatch the event manually, and dispatchEvent is synchronous call.

Comment 10

[Martijn Wargers (dead)]

Attachment: 900969.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=823464c5a331

Comment 11

[Martijn Wargers (dead)]

Attachment: 900969.diff

The failures are still happening in b2g, so this improvement didn't help for the failures.

I got the impression, I didn't get the failures on b2g when the tests would be carried out after page load, for some reason.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

What, we have the same test twice?

Comment 13

[Martijn Wargers (dead)]

Attachment: 900969.diff (for check-in)

(In reply to Olli Pettay [:smaug] from comment #12)
> What, we have the same test twice?

Yes, and there was already a test for devicemotion, so I didn't think there was a need to change it to a devicemotion test.

Please leave this bug open after this patch is checked in, the issue with this test not working on b2g is not resolved with this.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a54cdafcd916

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a54cdafcd916

Comment 16

[Martijn Wargers (dead)]

Thanks for the check-in, but this is not fixed for b2g yet.

Comment 17

[Masatoshi Kimura [:emk]]

You should have added [leave open] to the whiteboard. Otherwise the merge script will change the status automatically.

Comment 18

[Martijn Wargers (dead)]

Thanks for the info Masatoshi (I just saw jgriffin doing it, so I was just made aware of it).