Closed Bug 943705 Opened 6 years ago Closed 6 years ago

Create aggregated countdown timer in helpers.WaitHelper.waitForPageLoad

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: mcomella, Assigned: isurae)

References

Details

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

Attachments

(1 file, 5 obsolete files)

If we have multiple StateChangeVerifiers in waitForPageLoad, each one has it's on timeout counter. This is inconsistent depending on the number of verifiers.

Instead, there should be a global timer across all verifiers.
(Requested to be assigned to this bug in bug 946366 comment 1)

Same techniques as bug 946366 comment 2 apply here! Good luck!
Assignee: nobody → ashwinswaroop93
Status: NEW → ASSIGNED
Unassigning due to inactivity - let me know if you want to continue working on this.
Assignee: ashwinswaroop93 → nobody
Hi Michael,

Can I take this bug?
Sure, Isura!

As I mentioned in comment 1, bug 946366 is also similar, so let me know if you want to take a look at that as well. You might be able to abstract this functionality away to a helper function, and share it between the two pieces of code, so I recommend it!
Assignee: nobody → isurae
To clarify, do you mean to we should timeout if the total wait time exceeds CHANGE_WAIT_MS? So in 10 seconds, all the verifiers should return or timeout. Currently we wait 10 seconds per verifier.
Attached patch 943705.patch (obsolete) — Splinter Review
Attachment #8362625 - Flags: review?(michael.l.comella)
rnewman: I'm adding this file for the next patch in this bug, which only touches Robocop tests (making me question whether it even belongs in android-sync, but I wouldn't know where else to put it). Please review.

Isura: I googled around a bit and found a StopWatch class within the Apache Commons [1]. It's generally better to use tested library code than maintain a rewritten copy of such functionality yourself, so I'm adding it to the repo. I would ordinarily let you do this, however, it's a complicated process because it has to go through the our android background services repository [2], which has a completely different workflow. This will save you time and effort for things you likely don't need to learn how to set up (at least for now :P).

If you're curious about the process, let me know and I can elaborate some more.

[1]: https://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache/commons/lang/time/StopWatch.html
[2]: https://github.com/mozilla-services/android-sync/pull/390
Attachment #8363255 - Flags: review?(rnewman)
Misgivings:

* This is adding code to the main repo that seems to only be used in tests.
* The Apache Commons class is relatively heavyweight: it supports splits, throws exceptions, etc. etc. My feeling is that that outweighs any value of it being tested.
* It has a dependency on DurationFormatUtils, which you haven't imported. This rabbit-hole might be deep.

I would suggest simply deciding which timer you want (for this purpose, probably SystemClock.uptimeMillis, so we don't count deep sleep), and simply jam a `long` field into WaitHelper and check it directly.

Am I missing some reason why you need a more complex solution here?
Comment on attachment 8362625 [details] [diff] [review]
943705.patch

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

(In reply to Richard Newman [:rnewman] from comment #9)
> Am I missing some reason why you need a more complex solution here?

I suppose not.

Isura, do you want to try rnewman's suggestion in comment 9?

::: mobile/android/base/tests/helpers/WaitHelper.java
@@ +225,2 @@
>  }
> +

nit: Excess whitespace at the bottom of the file.
Attachment #8362625 - Flags: review?(michael.l.comella) → feedback+
Attachment #8363255 - Attachment is obsolete: true
Attachment #8363255 - Flags: review?(rnewman)
Michael,

Do you mean put a member variable in WaitHelper? Why can't we just use a local variable to track the time?
(In reply to Isura Edirisinghe from comment #11)
> Do you mean put a member variable in WaitHelper? Why can't we just use a
> local variable to track the time?

Oh, we can use a local variable. I specifically meant his suggestion not to use a timer class, when we really only need that one time value.

He also suggests using a different timer than System.currentTimeMillis. From the docs [1]: 

> This method shouldn't be used for measuring timeouts or other elapsed time measurements, as changing
> the system time can affect the results. Use nanoTime() for that.

rnewman suggests `SystemClock.uptimeMillis`, which seems reasonable to me. Take a look at SystemClock [2] for some others.

[1]: https://developer.android.com/reference/java/lang/System.html#currentTimeMillis%28%29
[2]: https://developer.android.com/reference/android/os/SystemClock.html
Attached patch 943705.patch (obsolete) — Splinter Review
Attachment #8362625 - Attachment is obsolete: true
Attachment #8364546 - Flags: review?(michael.l.comella)
Comment on attachment 8364546 [details] [diff] [review]
943705.patch

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

::: mobile/android/base/tests/helpers/WaitHelper.java
@@ +95,5 @@
>          titleEventExpecter.blockForEventDataWithTimeout(PAGE_LOAD_WAIT_MS);
>          titleEventExpecter.unregisterListener();
>  
> +        // Bug 943705 - The timeout wait time should be the aggregate time for all ChangeVerifiers.
> +        int remainingWaitTime = CHANGE_WAIT_MS;

nit: Rename `remainingWaitTime` to `remainingVerifierWaitMillis`

@@ +101,3 @@
>          // Verify remaining state has changed.
>          for (final ChangeVerifier verifier : pageLoadVerifiers) {
> +            final long startTime = SystemClock.uptimeMillis();

While this solution is correct, it could be more efficient if you retrieve the system clock value once outside of the loop (and thus change the `remainingWaitTime` assignment below).

nit: Rename `startTime` to `verifierStartMillis` (hint: if you need it!)

@@ +111,5 @@
>                  }
> +            }, remainingWaitTime);
> +
> +            remainingWaitTime -= (int)(SystemClock.uptimeMillis() - startTime);
> +            remainingWaitTime = Math.max(remainingWaitTime, 0);

This line (Math.max) should not be necessary - negative values will also time out. I tested this locally and also skimmed the source ([1] [2]).

[1]: https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Solo.java#L495
[2]: https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Waiter.java#L378
Attachment #8364546 - Flags: review?(michael.l.comella) → feedback+
Attached patch 943705.patch (obsolete) — Splinter Review
Simplified solution.
Attachment #8364546 - Attachment is obsolete: true
Attachment #8365161 - Flags: review?(michael.l.comella)
https://bugzilla.mozilla.org/show_bug.cgi?id=946366 
is quite similar. Should I apply the fix for that in this patch?
Comment on attachment 8365161 [details] [diff] [review]
943705.patch

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

Looks good to me.
Attachment #8365161 - Flags: review?(michael.l.comella) → review+
(In reply to Isura Edirisinghe from comment #16)
> https://bugzilla.mozilla.org/show_bug.cgi?id=946366 
> is quite similar. Should I apply the fix for that in this patch?

It's arguable, but since we already have two bugs filed and have the two separated, I would say leave them separated (it's easier to see distinct changes and backout that way, in case of danger). If it's annoying to manage them separately, e.g. one patch is built on top of the other so mercurial doesn't cause merge conflicts, you can declare a dependency - mark the dependent patch's bug as depending on the other, and explain the dependency in the associated comment.
(In reply to Michael Comella (:mcomella) from comment #17)
> Comment on attachment 8365161 [details] [diff] [review]
> 943705.patch
> 
> Review of attachment 8365161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.

Thanks Michael. Do I need to run this fix on the try servers before check-in? I imagine they might affect the test results.
That would not be a bad idea! Way to be proactive! :)

For the record, it's not necessary - it will get backed out from Fx-team if things break - but you can use your judgement on when it should or should not be necessary.

Do you have try access?
Comment on attachment 8365161 [details] [diff] [review]
943705.patch

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

Sorry, last minute nit (I noticed it from reviewing the other bug)!

::: mobile/android/base/tests/helpers/WaitHelper.java
@@ +94,5 @@
>          contentEventExpecter.unregisterListener();
>          titleEventExpecter.blockForEventDataWithTimeout(PAGE_LOAD_WAIT_MS);
>          titleEventExpecter.unregisterListener();
>  
> +        // Bug 943705 - The timeout wait time should be the aggregate time for all ChangeVerifiers.

nit: Actually, I don't think we need the bug number here - anyone interested can use `hg blame` to find out when the line was added.
Attached patch 943705.patch (obsolete) — Splinter Review
Removed bug number. Added temp variable for the wait time to improve readability.
Attachment #8365161 - Attachment is obsolete: true
Attachment #8365454 - Flags: review?(michael.l.comella)
Attached patch 943705.patchSplinter Review
Disregard previous attachment.
Attachment #8365454 - Attachment is obsolete: true
Attachment #8365454 - Flags: review?(michael.l.comella)
Attachment #8365455 - Flags: review?(michael.l.comella)
Comment on attachment 8365455 [details] [diff] [review]
943705.patch

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

Looks good to me! Thanks again!
Attachment #8365455 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #20)
> That would not be a bad idea! Way to be proactive! :)
> 
> For the record, it's not necessary - it will get backed out from Fx-team if
> things break - but you can use your judgement on when it should or should
> not be necessary.
> 
> Do you have try access?

I don't have try access.
Would you like to? :) I can vouch for your access!

You can follow the guide to getting commit access [1] and ping me when you need a voucher. You'll be looking at level 1 commit access to start (see the descriptions at [2]).

There is honestly no rush to get this patch in (i.e. it is not (yet) essential functionality and the code underneath it isn't changing much so it won't bitrot) so we can wait until you get access and you can push it yourself!

If you decide that you're not interested, I can also push it to try for you (but you should really go for it! ^_^).

[1]: https://www.mozilla.org/hacking/committer/
[2]: https://www.mozilla.org/hacking/commit-access-policy/
I have followed steps to get commit access. 

Please vouch for me. Thanks!

https://bugzilla.mozilla.org/show_bug.cgi?id=964956
It appears you're all set! Let's push to try: https://wiki.mozilla.org/ReleaseEngineering/TryServer !

Don't forget to push to try for bug 946366 too!
Flags: needinfo?(isurae)
ping! Do you need help pushing to try?
Hi Michael. Sorry for the delay! I've been quite busy. I will attempt to push to try for both bugs this week.
Flags: needinfo?(isurae)
No worries - take your time! I'm just concerned about your patches getting bitrot! :)
Hey, Isura.

Have you had a chance to work on this (and its sibling bug 946366)? Since pushing to Try isn't the most enjoyable thing in the world :P, if not, I'd be happy to push to try and land it for you if you're interested.
Flags: needinfo?(isurae)
(In reply to Michael Comella (:mcomella) from comment #32)
> Hey, Isura.
> 
> Have you had a chance to work on this (and its sibling bug 946366)? Since
> pushing to Try isn't the most enjoyable thing in the world :P, if not, I'd
> be happy to push to try and land it for you if you're interested.

Hi Michael. I have not been very active here recently. Could you push it to try for me? Next time I will do it myself. Thanks!
Flags: needinfo?(isurae)
https://hg.mozilla.org/mozilla-central/rev/51c6a235e621
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.