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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 1

[:Margaret Leibovic]

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.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

This sounds like an interesting task that I'd like to work on.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test.patch

Even though the ticket is not assigned to me (yet) I took the liberty of writing a first patch for it. :)

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Sebastian! Welcome back! :D

I'll take a look at this soon.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

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 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to push to try when the tree opens.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba057588e78

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test_revised.patch

Uploaded revised patch that fixes the nits.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test_revision2.patch

I refactored the test case to wait for the actual reader mode button and press it.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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).

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test_revisision3.patch

I updated the patch to address the mentioned points.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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!

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test_revision4.patch

(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.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1135111_reader_mode_title_test_revision5.patch

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?

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f8e958814354

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8e958814354

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sebastian, let me know if you want any followup bug suggestions.