1351667 - UI test that traverses settings screens

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1351295 introduced a crash in settings. Surprisingly we didn't catch this until we saw Nightly crashing. A test that just opens all settings screens should be pretty straight forward and catch things like this.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

@Gautam: You asked me for a Fennec bug to work on. Would you like to pick up this one?

This requires writing a robocop UI test:
https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#robocop_UI_tests

Have a look at robocop.ini to see some other tests and how they are structured. You can run them locally on an emulator (see wiki page). Never run the whole test suite (only single tests) - this takes ages and might even fail locally. :)

Comment 2

[Gautam Prajapati]

@Sebastian, Thanks. 
Sure, I'll take this up! :)

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Great!

Comment 4

[Gautam Prajapati]

Attachment: Bug 1351667 - UI test that traverses settings screens;

This commit adds a robocop test that traverses all the settings
 screens except the 'Make default browser' one.

Review commit: https://reviewboard.mozilla.org/r/125952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125952/

Comment 5

[Gautam Prajapati]

(In reply to Gautam Prajapati from comment #4)
> Created attachment 8853927 [details]
> Bug 1351667 - UI test that traverses settings screens;
> 
> This commit adds a robocop test that traverses all the settings
>  screens except the 'Make default browser' one.
> 
> Review commit: https://reviewboard.mozilla.org/r/125952/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/125952/

So, I added a simple test which traverses all settings sections(except 'Make Default Browser'), opens and goes back for every item. 

Make Default Browser is skipped because, in my opinion, it deserves an altogether new test as it is not same as testing other settings sections. 
It behaves differently for different Android versions. It opens a support URL for versions below Nougat and app settings screen above Nougat.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

https://reviewboard.mozilla.org/r/125952/#review129342

This is already pretty nice! If we add an additional assertion for the sub screens then this should be good to go!

I pushed the current version to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ec5158d4824

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testSettingsPages.java:17
(Diff revision 1)
> +
> +    public void testSettingsPages() {
> +        blockForGeckoReady();
> +
> +        // Open Settings Menu
> +        

nit: trailing white spaces (See mozreview, they are shown in bright red)

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testSettingsPages.java:41
(Diff revision 1)
> +        selectSettingsSection(mStringHelper.GENERAL_SECTION_LABEL);
> +        mSolo.goBack();

It would be nice if we add some check that we are inside the setting screen and display it. For example assert that a specific label is visible - e.g. for "General" we see "Language".

Comment 7

[Gautam Prajapati]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125952/diff/1-2/

Comment 8

[Gautam Prajapati]

(In reply to Gautam Prajapati from comment #7)
> Comment on attachment 8853927 [details]
> Bug 1351667 - UI test that traverses settings screens;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/125952/diff/1-2/

Thanks for reviewing Sebastian.

So I have added assertions to the test. Firstly we assert that we're inside Settings screen by checking if "General" label is present. 
Next, I have added assertions to each individual subsection, searching for a particular label in every subsection to make sure we are inside it.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

https://reviewboard.mozilla.org/r/125952/#review129836

Thanks this is looking good!

I'll trigger a new try run for this updated patch.

The previous run failed, can you try to investigate what went wrong?
> Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSettingsPages | Waiting for and scrolling once to find section ^Search$ - ^Search$ found

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ec5158d4824&selectedJob=88811196
Task Log: https://treeherder.mozilla.org/logviewer.html#?job_id=88811196&repo=try&lineNumber=1921
Device Log: https://public-artifacts.taskcluster.net/Ss5T8_TmR-K7DcRZuOIlYA/0/public/test_info//logcat-emulator-5554.log
Screenshot: https://public-artifacts.taskcluster.net/Ss5T8_TmR-K7DcRZuOIlYA/0/public/test_info//robocop-screenshot-org.mozilla.gecko.tests.testSettingsPages.jpg

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Try run of the new patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d72fb9f5095

Comment 11

[Gautam Prajapati]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8853927 [details]
> Bug 1351667 - UI test that traverses settings screens;
> 
> The previous run failed, can you try to investigate what went wrong?
> > Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSettingsPages | Waiting for and scrolling once to find section ^Search$ - ^Search$ found
> 
> Try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1ec5158d4824&selectedJob=88811196
> Task Log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=88811196&repo=try&lineNumber=1921
> Device Log:
> https://public-artifacts.taskcluster.net/Ss5T8_TmR-K7DcRZuOIlYA/0/public/
> test_info//logcat-emulator-5554.log
> Screenshot:
> https://public-artifacts.taskcluster.net/Ss5T8_TmR-K7DcRZuOIlYA/0/public/
> test_info//robocop-screenshot-org.mozilla.gecko.tests.testSettingsPages.jpg

This is strange, I'll wait for the new try run to finish and see if it fails similarly on API Level 15.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Often this can be the result of the test code moving quicker than the actual device (You want to click on something that is not yet there). Sometimes just a waitFor.... is helpful to make sure a screen transition has happened before moving on. You could also try running the test on an ARM emulator. They are usually pretty slow and might fail more often. :)

Comment 13

[Gautam Prajapati]

(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Often this can be the result of the test code moving quicker than the actual
> device (You want to click on something that is not yet there). Sometimes
> just a waitFor.... is helpful to make sure a screen transition has happened
> before moving on. You could also try running the test on an ARM emulator.
> They are usually pretty slow and might fail more often. :)

Hmm, Okay I'll try to run this test on API Level 15 ARM emulator. 
The new try run is on 'retry' status for some reason on API 15, what does it mean?

Comment 14

[Gautam Prajapati]

The retriggered try run failed at first assertion itself. I'm not sure what is causing the problem because I am not able to reproduce it locally. 

We're already using waitForPreferencesText() method in the assertion for it to wait, open and scroll/find the label passed to it. 

What do you suggest? :)

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

4 times the test passed and 2 times it failed.

Looking at the screenshots it seems we scrolled all the way to the bottom:
* https://public-artifacts.taskcluster.net/M70064HDSY-xBQaeRYNUGg/0/public/test_info//robocop-screenshot-org.mozilla.gecko.tests.testSettingsPages.jpg
* https://public-artifacts.taskcluster.net/SmmCj9rFQsq9O3n3pP0Zzg/0/public/test_info//robocop-screenshot-org.mozilla.gecko.tests.testSettingsPages.jpg

Did you have success running the tests locally on a slower/ARM emulator? Seeing this fail yourself will be helpful here.

One issue here could be that you are looking for "General" as soon as you clicked the menu item - without waiting for the screen transition. In this case the first search for "General" might fail (because settings hasn't opened yet) and then we try to scroll to find it but actually scroll out of the way. Try to find a way to assert that we are looking at the settings screen first and then start to look for the sections. Maybe have a look at other tests using waitForPreferencesText() - maybe you can use the same approach they use.

Comment 16

[Gautam Prajapati]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125952/diff/2-3/

Comment 17

[Gautam Prajapati]

Sorry, this took a while to be updated. My system doesn't seem to like emulators.

Could you please push the new patch for a try run? :)

I'm not sure if the problem is with my system or what, sometimes ARM 4.3 Emulator works good, passes the test, and the next time it is like really sluggish making itself very difficult to work with. When it is slow, it fails at different points on each test run.     

I could see one common problem - As I click on 'Settings' menu item, it transitions to the next screen successfully but the items are not enabled yet. Test failed at times due to this. 

So instead of asserting with waitForPreferencesText which scrolls out of the way, I decided to use waitForEnabledText which just waits for 'General' label to be visible and enabled for clicking. 

I noticed one more thing:

Please take a look at function 'selectSettingsItem' in BaseTest.java, it selects the 'Settings' menu item, then directly moves on to open the items in Settings screen and it is using waitForPreferencesText() to assert without waiting to know that it is inside settings screen or not.
 
I'm afraid to say the tests using this function for example 'testAboutPage.java' should also fail similarly at times on a slower ARM emulator. 

Anyway, problem of failing at first assertion in my test should be resolved now(The one where we check we're inside 'Settings'). 

Please push it for try run and suggest if it needs some changes. :)

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Here's the new try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8cbf0b2502a

Once completed I'll re-trigger the task running your test to see if it's failing randomly.

> I'm afraid to say the tests using this function for example 'testAboutPage.java' should also fail similarly at times on a slower ARM emulator.

That's absolutely possible. Unfortunately we have a lot of robocop tests that have issues and fail intermittently. Feel free to file a bug if you find such issues. Making tests more stable and avoiding re-runs is super helpful. :)

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, I re-triggered rc2 multiple times.

Btw. if you want to apply for commit access level 1 to be able to push to the try server then I'd vouch for you:
* https://wiki.mozilla.org/ReleaseEngineering/TryServer
* https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
* https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

https://reviewboard.mozilla.org/r/125952/#review133702

Looks like this time it doesn't fail. :)

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/StringHelper.java:228
(Diff revision 3)
> +
> +    // Clear Private Data Section
> +    public final String SITE_SETTINGS_LABEL;
> +
> +    // Mozilla Firefox Section
> +    public final String FAQs_LABEL;

nit: FAQs -> FAQS

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testSettingsPages.java:18
(Diff revision 3)
> +    public void testSettingsPages() {
> +        blockForGeckoReady();
> +
> +        // Open Settings Menu
> +        selectMenuItem(mStringHelper.SETTINGS_LABEL);
> +        

nit: trailing whitespaces

Comment 21

[Gautam Prajapati]

(In reply to Sebastian Kaspari (:sebastian) from comment #19)

> Btw. if you want to apply for commit access level 1 to be able to push to
> the try server then I'd vouch for you:

I already have Level 1 commit access. You vouched for me(Bug 1351575). :) 

I wasn't sure about the privileges it gave to me. I didn't know how to push patches to try server, I think I can do it now. :) 

Regarding the patch, I'll resolve the nitpicks asap. 

Thanks!

Comment 22

[Gautam Prajapati]

Comment on attachment 8853927 [details]
Bug 1351667 - UI test that traverses settings screens;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125952/diff/3-4/

Comment 23

[Gautam Prajapati]

(In reply to Gautam Prajapati from comment #22)
> Comment on attachment 8853927 [details]
> Bug 1351667 - UI test that traverses settings screens;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/125952/diff/3-4/

Sebastian, Did you get the chance to review the new patch? :)

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, the patch already has r+! I'll land it.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7f82843e6a8
UI test that traverses settings screens; r=sebastian

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Done! Thank your for your contribution! :)

Comment 27

[Carsten Book [:Tomcat]]

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