946366 - Aggregate timeout for events waited on in WaitHelper.waitForPageLoad

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

So PAGE_LOAD_WAIT_MS encompasses all events waited on.

This bug is similar to bug 943705, which does the same for the StateChangeVerifiers.

It also might be useful to refactor the code to store all of the events into a Collection and iterate over them, similar to the StateChangeVerifiers paradigm.

Comment 1

[ashwinswaroop93]

Is anybody working on this bug or can I take it up (and perhaps 943705)?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sure! I've assigned you to the bug so you can begin working on it.

Do you have your build environment set up? If not, follow the instructions at [1].

Since you're working in our Robocop test framework, you'll also want to follow the instructions at [2] to build and run the Robocop tests. Since you should only need to change Robocop, you'll only need to compile and install Fennec once, and just recompile Robocop for any changes you make.

Good luck and let me know if you have any questions! You can also ping me on IRC to get a quicker response - my nick is mcomella and I hang around in #mobile.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android
[2]: https://wiki.mozilla.org/Auto-tools/Projects/Robocop

Comment 3

[ashwinswaroop93]

When I try to run make mochitest-robocop, the following error occurs:

Traceback (most recent call last):
  File "_tests/testing/mochitest/runtestsremote.py", line 748, in <module>
    main()
  File "_tests/testing/mochitest/runtestsremote.py", line 552, in main
    dm = droid.DroidADB(deviceRoot=options.remoteTestRoot)
  File "/home/swaroop/mozilla/mozilla-central/obj-i686-pc-linux-gnu/_tests/testing/mochitest/devicemanagerADB.py", line 64, in __init__
    self._verifyDevice()
  File "/home/swaroop/mozilla/mozilla-central/obj-i686-pc-linux-gnu/_tests/testing/mochitest/devicemanagerADB.py", line 712, in _verifyDevice
    raise DMError("unable to connect to device")
devicemanager.DMError: unable to connect to device
make: *** [mochitest-robocop] Error 1

Any ideas on what to do?

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This line in particular jumps out at me:

> devicemanager.DMError: unable to connect to device

Are you using an Android device? I'm not sure if we support the emulator. Additionally, for the tests to run properly, your device has to be on the same network as your computer (which hosts a web server that the device connects to), though this fails at runtime.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey. Are you still working on this?

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Unassigning due to inactivity - let me know if you want to continue working on this.

Comment 7

[Isura Edirisinghe]

I'll take this one too.

Comment 8

[Isura Edirisinghe]

Attachment: 946366.patch

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8365238 [details] [diff] [review]
946366.patch

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

::: mobile/android/base/tests/helpers/WaitHelper.java
@@ +50,5 @@
>          sActions = context.getActions();
>  
>          sToolbar = (ToolbarComponent) context.getComponent(ComponentType.TOOLBAR);
> +
> +        sEventExpecters = new EventExpecter[] {

I don't think this refactor will work: `sActions.expectGeckoEvent` registers the listener [1] which gets unregistered each time `waitForPageLoad` is called (i.e. `expectGeckoEvent`, and its register call, must be called each time `waitForPageLoad` is called, to associate with the unregister calls).

[1]: https://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/FennecNativeActions.java?rev=8ab821223047#82

Comment 10

[Isura Edirisinghe]

Attachment: 946366.patch

Fixed implementation. I'm not sure whether creating the array each time is acceptable for performance. I was trying to remove some code duplication.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8365316 [details] [diff] [review]
946366.patch

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

Looks pretty good! I'd like the comment to be less ambiguous, but whether you want to change the rest of the nits are up to you (if you haven't noticed I tend to be pedantic so if you feel I am being so, feel free to ignore my nits :P)!

::: mobile/android/base/tests/helpers/WaitHelper.java
@@ +86,5 @@
>  
>          // Wait for the page load and title changed event.
> +        final EventExpecter[] eventExpecters = new EventExpecter[] {
> +            sActions.expectGeckoEvent("DOMContentLoaded"),
> +            sActions.expectGeckoEvent("DOMTitleChanged")

nit: To the hinderance of performance, I usually prefer the most readable code. In this case, I would have a `static final String[] whatever = new String[] {"DOMContentLoaded", ...};` as a member of the class and iterate over that to create the events here.

But I think it's also fine the way it is now (if we had to wait for more events, I would disagree though I think many other developers wouldn't mind - to each their own). I'm overly pedantic so your choice whether you want to change it or not! :) I just wanted to give my opinion.

As for performance, waitForPageLoad shouldn't get called frequently so slightly efficient less operations are acceptable. Additionally, this allocation is negligible compared to the event we're waiting for - page load - so we don't have to worry about it. Finally, testing does not need to be nearly as performant as the browser itself so there's another way you can take liberties. But again, thanks for being proactive!

@@ +92,4 @@
>  
>          initiatingAction.run();
>  
> +        // Bug 946366 - Aggregate the countdown timer.

"countdown" seems ambiguous: what are we counting down to? Also, what are we aggregating over?

nit: I don't think the bug number is necessary here. If they need the bug number, they can use `hg blame`.

@@ +94,5 @@
>  
> +        // Bug 946366 - Aggregate the countdown timer.
> +        final long expecterStartMillis = SystemClock.uptimeMillis();
> +        for (final EventExpecter expecter : eventExpecters) {
> +            expecter.blockForEventDataWithTimeout(PAGE_LOAD_WAIT_MS - (int)(SystemClock.uptimeMillis() - expecterStartMillis));

nit: Move the calculation into a temporary variable for readability - the line is a bit long and the extra variable name helps know what the calculation returns (we didn't do this in the other bug though we honestly could and maybe should have).

Comment 12

[Isura Edirisinghe]

Attachment: 946366.patch

Edit comment and add temporary variable for the wait time.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8365458 [details] [diff] [review]
946366.patch

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

Looks good to me!

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try: https://tbpl.mozilla.org/?tree=Try&rev=27602ccbb288

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/4253ca7dfe1a

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4253ca7dfe1a