Closed Bug 946366 Opened 9 years ago Closed 8 years ago

Aggregate timeout for events waited on in WaitHelper.waitForPageLoad


(Firefox for Android Graveyard :: Testing, defect)

Not set


(Not tracked)

Firefox 32


(Reporter: mcomella, Assigned: isurae)



(Whiteboard: [mentor=mcomella][lang=java])


(1 file, 2 obsolete files)

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.
Depends on: 910859
No longer depends on: 910589
Is anybody working on this bug or can I take it up (and perhaps 943705)?
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.

Assignee: nobody → ashwinswaroop93
When I try to run make mochitest-robocop, the following error occurs:

Traceback (most recent call last):
  File "_tests/testing/mochitest/", line 748, in <module>
  File "_tests/testing/mochitest/", line 552, in main
    dm = droid.DroidADB(deviceRoot=options.remoteTestRoot)
  File "/home/swaroop/mozilla/mozilla-central/obj-i686-pc-linux-gnu/_tests/testing/mochitest/", line 64, in __init__
  File "/home/swaroop/mozilla/mozilla-central/obj-i686-pc-linux-gnu/_tests/testing/mochitest/", 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?
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.
Hey. Are you still working on this?
Flags: needinfo?(ashwinswaroop93)
Unassigning due to inactivity - let me know if you want to continue working on this.
Assignee: ashwinswaroop93 → nobody
Flags: needinfo?(ashwinswaroop93)
I'll take this one too.
Assignee: nobody → isurae
Attached patch 946366.patch (obsolete) — Splinter Review
Attachment #8365238 - Flags: review?(michael.l.comella)
Comment on attachment 8365238 [details] [diff] [review]

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

::: mobile/android/base/tests/helpers/
@@ +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).

Attachment #8365238 - Flags: review?(michael.l.comella) → feedback+
Attached patch 946366.patch (obsolete) — Splinter Review
Fixed implementation. I'm not sure whether creating the array each time is acceptable for performance. I was trying to remove some code duplication.
Attachment #8365316 - Flags: review?(michael.l.comella)
Comment on attachment 8365316 [details] [diff] [review]

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/
@@ +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 @@
> +        // 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).
Attachment #8365316 - Flags: review?(michael.l.comella) → feedback+
Attached patch 946366.patchSplinter Review
Edit comment and add temporary variable for the wait time.
Attachment #8365238 - Attachment is obsolete: true
Attachment #8365316 - Attachment is obsolete: true
Attachment #8365458 - Flags: review?(michael.l.comella)
Comment on attachment 8365458 [details] [diff] [review]

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

Looks good to me!
Attachment #8365458 - Flags: review?(michael.l.comella) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.