Implement new test that ensure state is correct when switching to loading pages

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mcomella, Assigned: sebastian, Mentored)

Tracking

unspecified
Firefox 40
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 attachment, 5 obsolete attachments)

Bug 1124190 occurred because the state when switching to currently loading pages was inconsistent with the state when opening a page in the current tab. So this test should
  1) Open a link in a new tab
  2) Switch to the currently loading page (ensuring that the page will remain loading while we switch over to it - use timeouts to stall page loads, perhaps?)
  3) Assert the state is correct both while the page is still loading and when the page has finished loading

In particular, the state that should be asserted should at least match what bug 1124190 had issues with - the state of the back/forward buttons on new tablet after the page has finished loading should match the back/forward history of a new tab (i.e. all disabled). Additional state can be taken care of in a followup bug.

This new test should extend UITest. Don't forget to add your new test to robocop.ini! See [1] for more tips on writing new UITests.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android/UITest
Is this one good for me if I've never written a UI Test before? Could you mentor me, perhaps? :)
Never mind, I think I need a tablet to run the tests, right? If so, I'm sorry. :(
Hey, Alexander.

While this bug was initially filed as a followup to prevent regressions on tablet, it can still be completed without a tablet (i.e. the tablet work can be completed by someone else as a follow-up). If you're interested in working on it, let me know! You can also let me know if you'd like for me to suggest some bugs for you to work on!

Thanks!
Yes, please suggest some bugs! I'd be very thankful. :) I already know how to run robocop tests locally and I have my FF/Desktop and Mobile builds set. Thanks in advance! :)
(Spoke to Alexander on IRC)
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html
Whiteboard: [lang=java][good third bug] → [lang=java][good next bug]
Assignee: nobody → s.kaspari
I wrote a first (very hacky!) version of this test case. The test is asserting that the state of the back/forward buttons in correct. Of course this test case only works for tablets like this. Should this test case also cover phones? I assume the same bug could occur on phones inside the overflow menu... so this test should probably cover this too, right?

I'll need to clean up the code and move functionality from the test case (as I said it's hacky) to components.
Flags: needinfo?(michael.l.comella)
(In reply to :Sebastian Kaspari from comment #7)
> so this test should probably cover this too, right?

Sounds good to me, though if you'd rather take small steps - multiple patches or bugs - wfm.
Flags: needinfo?(michael.l.comella)
Status: NEW → ASSIGNED
It turns out you can't switch to the new (loading!) tab using the toast-like popup ("New tab opened | SWITCH")": After clicking on "SWITCH" the popup disappears but the browser does not switch to the new tab until the page has finished loading. I'm not sure if this can be considered a bug because the page loading is deferred using a busy-wait in JavaScript: It might be an acceptable side effect when you are blocking the execution (which is generally a very bad idea).

However tab switching works fine when clicking the tab counter and selecting the new tab. I'll refactor the test case to do this instead of using the popup.
(In reply to :Sebastian Kaspari from comment #9)
> but the browser does not switch to the new tab until the page has
> finished loading.

I'm not sure I understand why this is - can you elaborate?

> However tab switching works fine when clicking the tab counter and selecting
> the new tab. I'll refactor the test case to do this instead of using the
> popup.

I prefer using "switch" because that's what most users will use (thereby getting more test coverage), but if this works too, I'm fine with it.
(In reply to Michael Comella (:mcomella) from comment #10)
> I'm not sure I understand why this is - can you elaborate?

I'm not sure what exactly is causing this. I made a screen recording and uploaded it here:
https://www.youtube.com/watch?v=d4A5JFX9DCQ

I uploaded the test page here:
http://pocmo.de/mozilla/robocop_link_to_slow_loading.html

I can't reproduce this issue with other slow loading web pages so I assume that this is somehow caused by the busy-wait/loop in JavaScript.

> I prefer using "switch" because that's what most users will use (thereby
> getting more test coverage), but if this works too, I'm fine with it.

I'd prefer that too if I can somehow work around the issue. Clicking the tab counter and selecting the new tab works without any issues though.
(In reply to :Sebastian Kaspari from comment #11)
> I can't reproduce this issue with other slow loading web pages so I assume
> that this is somehow caused by the busy-wait/loop in JavaScript.

Notably, this also prevents me from switching tabs on desktop.

Perhaps hitting the "SWITCH" button sends a tab switch event to gecko while using the tab drawer doesn't wait for a response from Gecko (and thus doesn't block)?
(In reply to Michael Comella (:mcomella) from comment #12)
> Perhaps hitting the "SWITCH" button sends a tab switch event to gecko while
> using the tab drawer doesn't wait for a response from Gecko (and thus
> doesn't block)?

Ah, yeah, that makes sense. Do you think it's worth exploring this further in a new bug?
This is the current version of the patch. There are some things I'm not happy with:

* There's currently no way to limit a test to tablets in robocop.ini, right? Now I just perform an early return if !DeviceHelper.isTablet()

* The code to click the link is derived from ContentContextMenuTest. I would like to move this functionality to somewhere where it can be used from all tests - something like a ClickHelper class maybe?
Attachment #8574926 - Flags: feedback?(michael.l.comella)
(In reply to :Sebastian Kaspari from comment #13)
> Ah, yeah, that makes sense. Do you think it's worth exploring this further
> in a new bug?

Yeah, even without the test emphasis - it's crappy that some bad javascript can hang the browser chrome.
Comment on attachment 8574926 [details] [diff] [review]
1126048_testStateWhileLoading.patch

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

Looks like a reasonable approach to me.

(In reply to :Sebastian Kaspari from comment #14)
> * There's currently no way to limit a test to tablets in robocop.ini, right?
> Now I just perform an early return if !DeviceHelper.isTablet()

Not that I'm aware of - I've done the same thing in previous tests. If it ever gets fixed, it'll all get fixed together so I wouldn't worry about it.

> * The code to click the link is derived from ContentContextMenuTest. I would
> like to move this functionality to somewhere where it can be used from all
> tests - something like a ClickHelper class maybe?

Seems reasonable - I'd prefer "GeckoClickHelper" or similar though so we don't start using it to click everything. :)

::: mobile/android/base/tests/testStateWhileLoading.java
@@ +11,5 @@
> +
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;
> +
> +/**
> + * This test ensures the back/forward state is correct when switching to loading pages.

Add a comment mentioning that this is to prevent regressions like that one bug (including the bug #).

@@ +16,5 @@
> + */
> +public class testStateWhileLoading extends UITest {
> +    public void testStateWhileLoading() {
> +        if (!DeviceHelper.isTablet()) {
> +            // This test case only covers tablets.

*only currently

The intent is to eventually have this on phones too.

@@ +60,5 @@
> +        fAssertNotNull("Tab strip is not null", tabStrip);
> +        fAssertEquals("Tab strip contains two tabs", 2, tabStrip.getChildCount());
> +
> +        View tabView = tabStrip.getChildAt(1);
> +        fAssertNotNull("Second tab is not null", tabView);

This tab strip code should probably go in a TabStripComponent.
Attachment #8574926 - Flags: feedback?(michael.l.comella) → feedback+
I created bug 1143333 for analyzing the SWITCH button issue.
I attached the revised version of the patch.

Successful try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c1b100d3111
Attachment #8574926 - Attachment is obsolete: true
Attachment #8577718 - Flags: review?(michael.l.comella)
I created bug 1143393 as a follow-up to cover phones too.
Attachment #8577718 - Attachment is obsolete: true
Attachment #8577718 - Flags: review?(michael.l.comella)
Attachment #8577749 - Flags: review?(michael.l.comella)
Sorry for the delay, Sebastian - I'll be sure to get to this tomorrow.
Comment on attachment 8577749 [details] [diff] [review]
1126048_testStateWhileLoading_v3.patch

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

Nice!

The try run in comment 18 should suffice for a "checkin-needed". Thanks, Sebastian!

::: mobile/android/base/tests/UITest.java
@@ +41,5 @@
>      protected AboutHomeComponent mAboutHome;
>      protected AppMenuComponent mAppMenu;
>      protected GeckoViewComponent mGeckoView;
>      protected ToolbarComponent mToolbar;
> +    protected TabStripComponent mTabStrip;

nit: alphabetize.

@@ +94,5 @@
>          mAboutHome = new AboutHomeComponent(this);
>          mAppMenu = new AppMenuComponent(this);
>          mGeckoView = new GeckoViewComponent(this);
>          mToolbar = new ToolbarComponent(this);
> +        mTabStrip = new TabStripComponent(this);

nit: alphabetize.

::: mobile/android/base/tests/components/TabStripComponent.java
@@ +8,5 @@
> +
> +import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;
> +
> +/**
> + * A class representing any interactions that take place on the tab strip.

nit: "the Tablet tab strip."

@@ +19,5 @@
> +    public void switchToTab(int index) {
> +        // The tab strip is only available on tablets
> +        DeviceHelper.assertIsTablet();
> +
> +        TwoWayView tabStrip = (TwoWayView) mSolo.getView("tab_strip");

You can use R.id.tab_strip here by importing `org.mozilla.gecko.R`. Note that R is broken in my local build so it'll show as a compile error, but should run fine from mach.

nit: You should follow the conventions in the other components (i.e. `View getTabStripView()`).

::: mobile/android/base/tests/helpers/GeckoClickHelper.java
@@ +30,5 @@
> +     *
> +     * The link should be positioned at the top of the page, at least 60px high and
> +     * aligned to the middle.
> +     */
> +    public static void openLinkInNewTab() {

nit: The method name should indicate this isn't a generic method to open links and is for special-cases:

openCentralizedLinkInNewTab()?

::: mobile/android/base/tests/testStateWhileLoading.java
@@ +26,5 @@
> +                mTabStrip.switchToTab(1);
> +
> +                // Assert state of the back and forward button is correct after
> +                // switching to the new (still loading) tab.
> +                mToolbar.assertBackButtonIsNotEnabled();

Why not test the state of the forward button here? I'm fine doing it in a followup though.
Attachment #8577749 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #22)
> @@ +19,5 @@
> > +    public void switchToTab(int index) {
> > +        // The tab strip is only available on tablets
> > +        DeviceHelper.assertIsTablet();
> > +
> > +        TwoWayView tabStrip = (TwoWayView) mSolo.getView("tab_strip");
> 
> You can use R.id.tab_strip here by importing `org.mozilla.gecko.R`. Note
> that R is broken in my local build so it'll show as a compile error, but
> should run fine from mach.

That's what I did first and everything seemed to be fine but the Android 2.3 builds on try kept failing. I assume that for 2.3 builds all unrelated code gets stripped and the tab_strip id is only used in one tablet layout.
That's also the reason why I'm using the TwoWayView class here and not TabStripView.

> Why not test the state of the forward button here? I'm fine doing it in a
> followup though.

Good point. I think I omitted it because I realized that there's actually no forward button in this case. But of course that's a state I could assert too. I'll add it.
(In reply to :Sebastian Kaspari from comment #23)
> > You can use R.id.tab_strip here by importing `org.mozilla.gecko.R`. Note
> > that R is broken in my local build so it'll show as a compile error, but
> > should run fine from mach.
> 
> That's what I did first and everything seemed to be fine but the Android 2.3
> builds on try kept failing. I assume that for 2.3 builds all unrelated code
> gets stripped and the tab_strip id is only used in one tablet layout.
> That's also the reason why I'm using the TwoWayView class here and not
> TabStripView.

I see. Perhaps we can have a TabStripComponentStub that does not implement any methods and a TabStripComponentImpl that implements all of the methods, both inheriting from a TabStripComponent interface. versions > 2.3 will use the Impl, and all others will use the Stub. How does that sound?

If good, followup? You can be the primary mentor on this bug if you'd like (if not, just mark me). Mark it [good next bug] too.
(In reply to Michael Comella (:mcomella) from comment #24)
> I see. Perhaps we can have a TabStripComponentStub that does not implement
> any methods and a TabStripComponentImpl that implements all of the methods,
> both inheriting from a TabStripComponent interface. versions > 2.3 will use
> the Impl, and all others will use the Stub. How does that sound?
> 
> If good, followup? You can be the primary mentor on this bug if you'd like
> (if not, just mark me). Mark it [good next bug] too.

I like the idea of having two implementations. But I'm not sure how we can get around the compile error. TabStripComponentImpl would still be compiled for 2.3 try runs (and fail), right? Do you know an escape from that? Aside from that I wonder if TabStripComponentStub could raise wrong expectations: Why would a test call a method on TabStripComponent if it's not written to run on a tablet? Maybe it's good to potentially fail every access to the TabStripComponent by calling DeviceHelper.assertIsTablet() internally - just to notify the developer that the test may not make sense on every device?
(In reply to :Sebastian Kaspari from comment #25)
> TabStripComponentImpl would still be compiled
> for 2.3 try runs (and fail), right?

Oh, you're right - in the browser we can do this because we have to explicitly add all compiled files (and thus we add the "TabStrip" files to the files not compiled on 2.3.). Tests, on the other hand, add all files to the build.

Nick, is there any way we can exclude certain files from building on certain development configurations in the robocop APK? We want to exclude a file from building that references Tablet-only files when we're compiling a 2.3 build.

> Aside from that I wonder if TabStripComponentStub could raise wrong expectations:
> Why would a test call a method on TabStripComponent if it's not written to
> run on a tablet? Maybe it's good to potentially fail every access to the
> TabStripComponent by calling DeviceHelper.assertIsTablet() internally - just
> to notify the developer that the test may not make sense on every device?

Sounds good to me.
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #26)
> (In reply to :Sebastian Kaspari from comment #25)
> > TabStripComponentImpl would still be compiled
> > for 2.3 try runs (and fail), right?
> 
> Oh, you're right - in the browser we can do this because we have to
> explicitly add all compiled files (and thus we add the "TabStrip" files to
> the files not compiled on 2.3.). Tests, on the other hand, add all files to
> the build.
> 
> Nick, is there any way we can exclude certain files from building on certain
> development configurations in the robocop APK? We want to exclude a file
> from building that references Tablet-only files when we're compiling a 2.3
> build.

We could do like Fennec [1] in Robocop's moz.build... except Robocop is still on Makefile.in.  Sigh.  This would be improved by Bug 938659, if we transitioned to moz.build as part of that ticket; but it could also just be done in Robocop's Makefile.in, at or around [2].  I tried to make this work but comparing integers in Make is tricky and I didn't figure it out; glandium could certainly help.  Sorry that this isn't easy :(  If it's important, pick up some of my old patches for moving Robocop to moz.build and I could mentor.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#560

[2] https://dxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Makefile.in#41
Flags: needinfo?(nalexander)
I attached a new patch that addresses the nits.

I have been running into problems with asserting the state of the forward button (Somehow I can find a forward button and it's visible and enabled) so I created a follow up bug to extend the test case: bug 1148919.

Moving Robocop to moz.build sounds like an interesting task (even though I just saw moz.build for the first time). Maybe I can help with that as soon as I have more time. Is there already a bug for this task somewhere?
Attachment #8577749 - Attachment is obsolete: true
Attachment #8585180 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/7b1cb7256f0f
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=java][good next bug] → [lang=java][good next bug][fixed-in-fx-team]
(In reply to :Sebastian Kaspari from comment #28)
> I have been running into problems with asserting the state of the forward
> button (Somehow I can find a forward button and it's visible and enabled) so
> I created a follow up bug to extend the test case: bug 1148919.

It's never a dull day, is it? :)

Thanks again!

> Moving Robocop to moz.build sounds like an interesting task (even though I
> just saw moz.build for the first time). Maybe I can help with that as soon
> as I have more time. Is there already a bug for this task somewhere?

Nick?
Flags: needinfo?(nalexander)
Backed out for robocop failures.
https://hg.mozilla.org/integration/fx-team/rev/8c43ae08a6c8

https://treeherder.mozilla.org/logviewer.html#?job_id=2477514&repo=fx-team
Whiteboard: [lang=java][good next bug][fixed-in-fx-team] → [lang=java][good next bug]
Sometimes I wish I could slow down the devices to catch these things..

After clicking on "Open in new tab" the test case instantly checks whether it can find the new tab and switch to it. I probably have to waitFor() the new tab to appear and then continue to click it.

Looking at the code it seems like there's a screenshot made whenever a test case fails. Is it possible to grab this screenshot from treeherder? It would help me to verify my assumption.
(In reply to :Sebastian Kaspari from comment #32)
> Looking at the code it seems like there's a screenshot made whenever a test
> case fails. Is it possible to grab this screenshot from treeherder? It would
> help me to verify my assumption.

If you open the raw log and find-in-page for "screenshot", it'll take you to a base64 blob. Easiest way to view that is to copy-pasta it into the url bar of your browser (with the scheme). :P
Refactored the patch. TabStripComponent.switchToTab(index) now waits for the tab to appear instead of assuming the tab is already there. In addition to that I had to replace the StringHelper constants with a call to StringHelper.get().

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7dca0c1d72d

rc4 failed once on "Android 4.3 API11+ opt" but this was caused by an other test (testAppMenuPathways). The retriggered job passed without any failures.
Attachment #8585180 - Attachment is obsolete: true
Attachment #8597700 - Flags: review?(michael.l.comella)
Comment on attachment 8597700 [details] [diff] [review]
1126048_testStateWhileLoading_v5.patch

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

::: mobile/android/base/tests/testStateWhileLoading.java
@@ +18,5 @@
> +        }
> +
> +        GeckoHelper.blockForReady();
> +
> +        NavigationHelper.enterAndLoadUrl(StringHelper.get().ROBOCOP_LINK_TO_SLOW_LOADING);

I don't understand - why can't you use mStringHelper here?
(In reply to Michael Comella (:mcomella) from comment #35)
> I don't understand - why can't you use mStringHelper here?

Oh! I have been looking for a StringHelper instance in UITest but obviously haven't checked BaseRobocopTest. :)
Comment on attachment 8597700 [details] [diff] [review]
1126048_testStateWhileLoading_v5.patch

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

Looks good - thanks, Sebastian!

::: mobile/android/base/tests/robocop_slow_loading.html
@@ +9,5 @@
> +        var start = new Date();
> +        var now = null;
> +        do {
> +            now = new Date();
> +        } while (now - start < 10000);

nit: Make 10000 a temporary variable (e.g. like a constant) near the comment saying "Busy wait". I'd also leave the actual time out at that point then because it's easy to read from the following line and doesn't require you to remember to update two values. Include units in the variable name, e.g. WAIT_MILLIS.

::: mobile/android/base/tests/testStateWhileLoading.java
@@ +18,5 @@
> +        }
> +
> +        GeckoHelper.blockForReady();
> +
> +        NavigationHelper.enterAndLoadUrl(StringHelper.get().ROBOCOP_LINK_TO_SLOW_LOADING);

Okay, use mStringHelper as I mentioned. :)
Attachment #8597700 - Flags: review?(michael.l.comella) → review+
Here's the new patch that addresses the nits!
Attachment #8597700 - Attachment is obsolete: true
Attachment #8600334 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ca3c11c3b0bf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(In reply to Michael Comella (:mcomella) from comment #30)
> (In reply to :Sebastian Kaspari from comment #28)
> > I have been running into problems with asserting the state of the forward
> > button (Somehow I can find a forward button and it's visible and enabled) so
> > I created a follow up bug to extend the test case: bug 1148919.
> 
> It's never a dull day, is it? :)
> 
> Thanks again!
> 
> > Moving Robocop to moz.build sounds like an interesting task (even though I
> > just saw moz.build for the first time). Maybe I can help with that as soon
> > as I have more time. Is there already a bug for this task somewhere?
> 
> Nick?

I was tracking this in Bug 938659, but I camped that ticket for a simpler change.  There's WIP on changing the build system in that ticket, I think.  I don't know why I didn't finish Bug 929298.  Bug 994135 is relevant.  I swear I did this somewhere and never pushed it to completion, but now I can't find it :(

This is not an easy task, but I can help mentor.  File a ticket!
Flags: needinfo?(nalexander)
You need to log in before you can comment on or make changes to this bug.