Get session store robocop tests working again

RESOLVED FIXED in Firefox 55

Status

()

Firefox for Android
Testing
P3
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

Trunk
Firefox 55
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Assignee)

Description

11 months ago
Prompted by bug 1346634 - we should get these working again, so things like that won't go undetected again.

Updated

11 months ago
Priority: -- → P3
(Assignee)

Comment 1

11 months ago
After converting those tests to be UITest based, a try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=76d6e393f90c52d945488f27e2c34092308f1ee2) looks promising, so on that basis I'd tend to go ahead.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8848793 [details]
Bug 1348086 - Part 0 - Fix javadoc error.

https://reviewboard.mozilla.org/r/121674/#review123982
Attachment #8848793 - Flags: review?(gbrown) → review+

Comment 11

11 months ago
mozreview-review
Comment on attachment 8848794 [details]
Bug 1348086 - Part 1 - Use distinct R.id for the tabs tray menu button.

https://reviewboard.mozilla.org/r/121676/#review123984

This seems like a really good find - thanks!
Attachment #8848794 - Flags: review?(gbrown) → review+

Comment 12

11 months ago
mozreview-review
Comment on attachment 8848795 [details]
Bug 1348086 - Part 2 - Check whether an URL is already absolute before processing it.

https://reviewboard.mozilla.org/r/121678/#review123986

Wouldn't the caller typically know that it doesn't need to call getAbsoluteUrl() then? But if it's convenient this way, I don't object.
Attachment #8848795 - Flags: review?(gbrown) → review+
(Assignee)

Comment 13

11 months ago
mozreview-review-reply
Comment on attachment 8848795 [details]
Bug 1348086 - Part 2 - Check whether an URL is already absolute before processing it.

https://reviewboard.mozilla.org/r/121678/#review123986

Ordinarily yes. But here it's `ToolbarComponent.assertTitle()` that's calling this, so nothing that's immediately within my test's control. Allowing that function to be called with absolute URLs as well seemed easier than having to rewrite SessionTest to store relative URLs which then have to be made absolute again in all sorts of different places around the test.

Comment 14

11 months ago
mozreview-review
Comment on attachment 8848797 [details]
Bug 1348086 - Part 4 - Port verifyTabCount() to ToolbarComponent.

https://reviewboard.mozilla.org/r/121682/#review124186

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/ToolbarComponent.java:126
(Diff revision 1)
>      private TextView getUrlTitleText() {
>          return (TextView) getToolbarView().findViewById(R.id.url_bar_title);
>      }
>  
> +    private Element getTabsCounter() {
> +        return mDriver.findElement(mTestContext.getActivity(), R.id.tabs_counter);

I don't know much about the UITest branch of robocop tests, but I think they generally do not use Element/Driver/findElement(). Could you not use something like findViewById or perhaps waitForView/getView?
Attachment #8848797 - Flags: review?(gbrown) → review-

Comment 15

11 months ago
mozreview-review
Comment on attachment 8848796 [details]
Bug 1348086 - Part 3 - Port selectTabAt() to TabsPanelComponent.

https://reviewboard.mozilla.org/r/121680/#review124194

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/TabsPanelComponent.java:18
(Diff revision 1)
>  import java.util.List;
>  
>  import static org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertTrue;
>  
>  public class TabsPanelComponent extends TabsPresenterComponent {
> +    private static final int MAX_WAIT_MS = 4500;

Why 4500? Probably trial-and-error until it passes? That's fine, but I would encourage you to leave plenty of extra time to allow for a changing environment, even slower device/emulator, etc.
Attachment #8848796 - Flags: review?(gbrown) → review+

Comment 16

11 months ago
mozreview-review
Comment on attachment 8848799 [details]
Bug 1348086 - Part 6 - Remove selectTabAt() from BaseTest.

https://reviewboard.mozilla.org/r/121686/#review124196
Attachment #8848799 - Flags: review?(gbrown) → review+

Comment 17

11 months ago
mozreview-review
Comment on attachment 8848798 [details]
Bug 1348086 - Part 5 - Convert session store robocop tests to be based on UITest.

https://reviewboard.mozilla.org/r/121684/#review124198

Nice! Thanks.
Attachment #8848798 - Flags: review?(gbrown) → review+

Comment 18

11 months ago
mozreview-review
Comment on attachment 8848800 [details]
Bug 1348086 - Part 7 - Reenable Robocop session store tests.

https://reviewboard.mozilla.org/r/121688/#review124200

It will be great to see these running again after all this time.

I think there's only the one small issue in part 4 to address. Please push to try again after addressing that issue.
Attachment #8848800 - Flags: review?(gbrown) → review+
(Assignee)

Comment 19

11 months ago
mozreview-review-reply
Comment on attachment 8848797 [details]
Bug 1348086 - Part 4 - Port verifyTabCount() to ToolbarComponent.

https://reviewboard.mozilla.org/r/121682/#review124186

> I don't know much about the UITest branch of robocop tests, but I think they generally do not use Element/Driver/findElement(). Could you not use something like findViewById or perhaps waitForView/getView?

The problem was not so much finding the view itself, but rather subsequently getting the tab count out of the view. I'll look into what `Element.getText()` actually does under the hood, though...
(Assignee)

Comment 20

11 months ago
mozreview-review-reply
Comment on attachment 8848796 [details]
Bug 1348086 - Part 3 - Port selectTabAt() to TabsPanelComponent.

https://reviewboard.mozilla.org/r/121680/#review124194

> Why 4500? Probably trial-and-error until it passes? That's fine, but I would encourage you to leave plenty of extra time to allow for a changing environment, even slower device/emulator, etc.

I just copied this from the old BaseTest code and didn't give this much further though, to be honest. A number of try runs were okay, though and I've just tested it locally with 1/10th the value and at least for this test this was still working fine.

Comment 21

11 months ago
OK, sounds good enough then!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

11 months ago
mozreview-review
Comment on attachment 8848797 [details]
Bug 1348086 - Part 4 - Port verifyTabCount() to ToolbarComponent.

https://reviewboard.mozilla.org/r/121682/#review125124
Attachment #8848797 - Flags: review?(gbrown) → review+

Comment 31

11 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/fe5cf6080e6f
Part 0 - Fix javadoc error. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/f82206bad1ee
Part 1 - Use distinct R.id for the tabs tray menu button. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/fdf2be0e543b
Part 2 - Check whether an URL is already absolute before processing it. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/7126af637c33
Part 3 - Port selectTabAt() to TabsPanelComponent. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/62f8400ad089
Part 4 - Port verifyTabCount() to ToolbarComponent. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/0145a8c1f594
Part 5 - Convert session store robocop tests to be based on UITest. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/c493762b15f5
Part 6 - Remove selectTabAt() from BaseTest. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/ab4053092156
Part 7 - Reenable Robocop session store tests. r=gbrown

Updated

9 months ago
Depends on: 1361924
You need to log in before you can comment on or make changes to this bug.