Closed Bug 1135111 Opened 5 years ago Closed 5 years ago

Add test to ensure that the toolbar in reader mode displays the original page url

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: mcomella, Assigned: sebastian, Mentored)

References

Details

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

Attachments

(1 file, 5 obsolete files)

After bug 1123904 lands, about:reader urls will display as the original page url. Add a test to verify this works.

NI self to verify how to make a test page that is reader mode compatible.
Flags: needinfo?(michael.l.comella)
For testReadingListCache, we already have some test articles in the tree:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/reader_mode_pages/

We could probably just load about:reader?url=.../basic_article.html in a tab and then verify what URL is displayed.
(In reply to :Margaret Leibovic from comment #1)
> We could probably just load about:reader?url=.../basic_article.html in a tab
> and then verify what URL is displayed.

Sounds good to me.
Flags: needinfo?(michael.l.comella)
Whiteboard: [lang=java] → [lang=java][good next bug]
This sounds like an interesting task that I'd like to work on.
Even though the ticket is not assigned to me (yet) I took the liberty of writing a first patch for it. :)
Attachment #8567982 - Flags: review?(michael.l.comella)
Hey, Sebastian! Welcome back! :D

I'll take a look at this soon.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Hey, Michael! It's good to be back. :)

The test ran successfully on the following devices so far:
* Nexus 5 (Android 5.0.1)
* Nexus 7 (2012, Android 5.0.2)
* HTC Desire HD (Android 2.3.5)
Comment on attachment 8567982 [details] [diff] [review]
1135111_reader_mode_title_test.patch

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

lgtm w/ nits.

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +113,5 @@
>          DeviceHelper.assertIsTablet();
>          return (ImageButton) getToolbarView().findViewById(R.id.reload);
>      }
> +
> +    private View getReaderModeButton() {

This (and press*) should be *PageActionLayout (e.g. getPageActionLayout). We put other items in the page action layout so it could be confusing when pressReaderModeButton opens the Youtube app, or similar.

::: mobile/android/base/tests/robocop.ini
@@ +151,5 @@
>  skip-if = android_version == "10"
>  [testJavascriptBridge]
>  [testNativeCrypto]
>  [testSessionHistory]
> +[testReaderModeTitle]

nit: alphabetize

::: mobile/android/base/tests/testReaderModeTitle.java
@@ +6,5 @@
> +public class testReaderModeTitle extends UITest {
> +    public void testReaderModeTitle() {
> +        mToolbar.enterEditingMode();
> +        mToolbar.enterUrl(getAbsoluteHostnameUrl(StringHelper.ROBOCOP_READER_MODE_BASIC_ARTICLE));
> +        mToolbar.commitEditingMode();

NavigationHelper.enterAndLoadUrl(...)

@@ +8,5 @@
> +        mToolbar.enterEditingMode();
> +        mToolbar.enterUrl(getAbsoluteHostnameUrl(StringHelper.ROBOCOP_READER_MODE_BASIC_ARTICLE));
> +        mToolbar.commitEditingMode();
> +
> +        mToolbar.pressReaderModeButton();

As per my other comment, add a note here that tapping the page action layout doesn't actually guarantee reader mode opening if there are other items in the page action layout.
Attachment #8567982 - Flags: review?(michael.l.comella) → review+
NI self to push to try when the tree opens.
Flags: needinfo?(michael.l.comella)
Uploaded revised patch that fixes the nits.
Attachment #8567982 - Attachment is obsolete: true
Attachment #8568740 - Flags: review?(michael.l.comella)
Comment on attachment 8568740 [details] [diff] [review]
1135111_reader_mode_title_test_revised.patch

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

lgtm, given green try.
Attachment #8568740 - Flags: review?(michael.l.comella) → review+
The test failed for one configuration because the PageActionLayout's visibility was "gone". Probably a timing issue. Retesting this on a slow emulator revealed another problem: Sometimes the (visible) PageActionLayout is pressed before the "Reader mode" button is added. Of course after this the page title is correct (but we are not in reader mode) resulting in a false positive test. Instead of blindly pressing the PageActionLayout the test has to wait until the actual reader mode button has appeared.
I refactored the test case to wait for the actual reader mode button and press it.
Attachment #8568740 - Attachment is obsolete: true
Attachment #8570677 - Flags: review?(michael.l.comella)
Comment on attachment 8570677 [details] [diff] [review]
1135111_reader_mode_title_test_revision2.patch

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

lgtm w/ green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7d1b135d453

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +122,5 @@
> +    private ImageButton getReaderModeButton() {
> +        final PageActionLayout pageActionLayout = getPageActionLayout();
> +        final int count = pageActionLayout.getChildCount();
> +
> +        for (int i = 0; i< count; i++) {

nit: i < count

@@ +288,5 @@
>          });
>      }
>  
> +    private void waitForReaderModeButton() {
> +        WaitHelper.waitFor("Reader mode button is visible", new Condition() {

nit: "the Reader mode button to be visible"

WaitHelper.waitFor forms a sentence with waitFor. :)

@@ +291,5 @@
> +    private void waitForReaderModeButton() {
> +        WaitHelper.waitFor("Reader mode button is visible", new Condition() {
> +            @Override
> +            public boolean isSatisfied() {
> +                return getReaderModeButton() != null;

While not necessary, it'd be good to return the non-null value of getReaderModeButton() from this method for consistency (though I suppose if we can't find it again, we're really hosed anyway).
Attachment #8570677 - Flags: review?(michael.l.comella) → review+
I updated the patch to address the mentioned points.
Attachment #8570677 - Attachment is obsolete: true
Attachment #8571639 - Flags: review?(michael.l.comella)
I wonder why the test failed on "Android 2.3 API9 opt" with "TEST-UNEXPECTED-FAIL | testReaderModeTitle | Waiting for Reader mode button is visible".

I ran the test successfully on a HTC Desire HD (Android 2.3.5) here. I can only imagine that the emulator is so slow that it takes quite some time until the reader mode button appears. Then again the default maximum waiting time is 15 seconds - that's plenty of time. However looking at bugs like bug 1114655 maybe 15 seconds is really not enough?
(In reply to :Sebastian Kaspari from comment #16)
> I ran the test successfully on a HTC Desire HD (Android 2.3.5) here. I can
> only imagine that the emulator is so slow that it takes quite some time
> until the reader mode button appears.

I remember back in the day on slower devices that the reader mode parsing took a noticeable amount of time - I doubt it's changed much so I think it's somewhat expensive.

> However looking at bugs like bug
> 1114655 maybe 15 seconds is really not enough?

Do you have try access? We could try throwing it on try with a timeout of 30 seconds or so!
(In reply to Michael Comella (:mcomella) from comment #17)
> Do you have try access? We could try throwing it on try with a timeout of 30
> seconds or so!

No, I don't have access yet. If I apply for Level 1 access, would you vouch for me?

Anyways I updated the code to wait 30 seconds - hopefully this will be enough time for the 2.3 emulator.
Attachment #8571639 - Attachment is obsolete: true
Attachment #8571639 - Flags: review?(michael.l.comella)
Attachment #8574095 - Flags: review?(michael.l.comella)
(In reply to :Sebastian Kaspari from comment #18)
> No, I don't have access yet. If I apply for Level 1 access, would you vouch
> for me?

Of course!

Lazy link: https://www.mozilla.org/en-US/about/governance/policies/commit/ :)

> Anyways I updated the code to wait 30 seconds - hopefully this will be
> enough time for the 2.3 emulator.

Let's find out! https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e3396eb2a4
Comment on attachment 8574095 [details] [diff] [review]
1135111_reader_mode_title_test_revision4.patch

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

lgtm w/ green try.

By the way, if I give you an r+ w/ nits, there is no need to re-request review - you can just address the review comments (whether fixing them or saying that you don't think it's a good idea) and upload the patch.

Thanks for your help! :)

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +295,5 @@
> +            @Override
> +            public boolean isSatisfied() {
> +                return (readerModeButton[0] = getReaderModeButton()) != null;
> +            }
> +        }, 30000);

nit: constant.
Attachment #8574095 - Flags: review?(michael.l.comella) → review+
Awesome, now the test passed! I uploaded an updated version of the patch adding the constant.

(In reply to Michael Comella (:mcomella) from comment #20)
> By the way, if I give you an r+ w/ nits, there is no need to re-request
> review - you can just address the review comments (whether fixing them or
> saying that you don't think it's a good idea) and upload the patch.

Alright! Do I need to take care of preserving the review+ flag somehow? Or is it enough to just add the checkin-needed keyword as soon as everything is done?
Attachment #8574095 - Attachment is obsolete: true
(In reply to :Sebastian Kaspari from comment #21)
> Alright! Do I need to take care of preserving the review+ flag somehow?

It is my impression that you should upload the revised patch and mark it r+ - no one ever complained to me about doing it. :P

> Or is it enough to just add the checkin-needed keyword as soon as everything is
> done?

Once you add the r+, add the checkin-needed keyword.
Attachment #8574425 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/f8e958814354
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=java][good next bug] → [lang=java][good next bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f8e958814354
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good next bug][fixed-in-fx-team] → [lang=java][good next bug]
Target Milestone: --- → Firefox 39
Depends on: 1142699
Sebastian, let me know if you want any followup bug suggestions.
You need to log in before you can comment on or make changes to this bug.