874985 - Create session restore unit tests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

The session restore code is fragile and seems to regress every few months, so we should create some tests cases.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: WIP session restore test case

Here's a WIP patch that tests session restore behavior. It works by creating two separate tests. The first is responsible for building tabs and saving the browser state; the second loads the browser state and verifies that the session is correctly restored.

It feels hacky to require multiple connected tests that run back-to-back like this, but I've unsuccessfully tried a few alternatives:

1) I tried having multiple testX methods in a single test file that run successively; however, since it's the same ActivityTestCase instance, the entire application isn't killed between each run, making it difficult to test restore behavior. I tried killing the application process after running the first test method, but that ended up killing the entire test framework.

2) I also tried extending from ActivityUnitTestCase instead of ActivityInstrumentationTestCase2 so that I could manually control the activity's lifecycle, but Fennec didn't play well with the mock application environment. Also, since the entire activity runs in a sort of simulated environment (and isn't shown on the screen), I'm not sure we'd be able to use Solo with this approach.

Any other ideas welcome.

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 752856 [details] [diff] [review]
WIP session restore test case

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

I share your concerns and frustration, but I think you are using the right approach here. You want to test behavior across a Fennec restart -- 2 separate robocop tests are the "right" way to simulate that.

One way to decouple the tests:
 - have one test verify that the session store file is created and has the expected contents
 - have the other test restore from a static file (copied from robocop assets or something like that) and verify that the expected tabs are restored

...but I think that approach is fragile in its own way.

::: mobile/android/base/tests/SessionTest.java.in
@@ +141,5 @@
> +                copyFile(file, new File(mProfile, filename));
> +                return true;
> +            }
> +        } catch (IOException e) {
> +            mAsserter.ok(false, "Error backing up " + filename, e.toString());

s/backing up/restoring/

::: mobile/android/base/tests/testSessionRestoreOOM1.java.in
@@ +34,5 @@
> +        addTab("http://www.wikipedia.org");
> +        loadUrl("http://www.yahoo.com");
> +
> +        // sessionstore.js is written to asynchronously, so wait before copying it
> +        mSolo.sleep(10000);

Could you:
 - wait for an event, or
 - poll for changing file size?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

I really don't like the idea of test cases depending on each other.  At the very least can we craft testcase 2 to terminate early with no checks if the data from testcase 1 is not found?

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Joel Maher (:jmaher) from comment #3)
> I really don't like the idea of test cases depending on each other.  At the
> very least can we craft testcase 2 to terminate early with no checks if the
> data from testcase 1 is not found?

I've been debating whether running back-to-back test cases is better than the JSON validation that Geoff mentioned in comment 2.

One the one hand, serializing the data and using it between tests means we don't need to care about the actual internal format of the bundle or session data; any new bundle entries we add or changes to the session store will work automatically. The downside, of course, is that we have test dependencies.

On the other hand, by checking and feeding the tests expected JSON data, the tests won't depend on each other. But one concern here is that since the actual bundle/session data from the application isn't being used, the test cases may miss bugs introduced by new entries. For example, if someone introduces a bundle entry to GeckoApp#onSaveInstanceState() that ends up breaking session restore, we won't detect the bug since our predefined JSON won't have automatically have the new entry. It's also inconvenient to have to change all of our stored JSON test data whenever something changes. But maybe this is all too contrived for it to matter much in practice; I don't know if our session/bundle data will really change that much in the future. I'll go ahead with this option so the tests can be run independently.

Comment 5

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Robustify existing test methods

A few changes to the existing test framework that seemed helpful. When verifying the URL, this clicks the back button afterwards to restore the UI to the state it was before the method was called. I also added event expecters for back/forward navigation to prevent races during repeated back/forward calls.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add stateBundle extra to allow injecting fake state bundles

We can't manipulate the savedInstanceState bundle given to GeckoApp in onCreate() since it's controlled by the OS, so we need another way to feed in a bundle. This patch uses an intent extra to emulate a bundle given to onCreate().

One concern I had was that exposing this meant other apps could use this intent to maliciously manipulate the bundle, so I protected it using a private pref.

Also removed useless code we have in onSaveInstanceState() (the given outState can never be null).

Comment 7

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Session restore tests

Session restore unit tests. I abstracted the session creation/verification logic into a SessionTest class since I plan on creating other session-related tests (tests for private data, non-OOM restores, etc.).

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 756867 [details] [diff] [review]
Part 2: Add stateBundle extra to allow injecting fake state bundles

I'm not a fan of putting test only code in the shipping app, but Brian talked me into this change.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 756869 [details] [diff] [review]
Part 3: Session restore tests

I don't feel qualified enough for an r+, but the general idea looks good. The changes/additions to the testing utilities seem good.

SessionTest might be a bit complicated for my taste, but as long as it works *reliably* I'm OK with it. You probably have some Try runs, but make sure you re-trigger the rc runs several times to see if we get solid green or intermittent fails.

I'll defer to Geoff for the core review.

Comment 10

[Brian Nicholson (:bnicholson)]

Try runs here: https://tbpl.mozilla.org/?tree=Try&rev=4d171920dd00

Looks good on 2.2, but there seem to be some issues on Android 4.0:

8 INFO TEST-UNEXPECTED-FAIL | testSessionRestoreOOMSave | Exception caught - junit.framework.AssertionFailedError: 7 INFO TEST-UNEXPECTED-FAIL | testSessionRestoreOOMSave | Awesomebar URL typed properly - got http://mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page1, expected http://mochi.test:8888/tests/robocop/robocop_dynamic.sjs?id=page2

Not sure why the URL wouldn't be typed properly, but this may be related to the back key that I added to the end of verifyURL(). I'll have to investigate more.

Comment 11

[Brian Nicholson (:bnicholson)]

Filed bug 879505 for the "Awesomebar URL typed properly" since I believe the issue isn't directly related to these patches.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 756865 [details] [diff] [review]
Part 1: Robustify existing test methods

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

I'm not sure that changes are required here for an r+, but let's discuss these points and then re-visit the review.

::: mobile/android/base/tests/BaseTest.java.in
@@ +275,4 @@
>              urlbarText = urlbar.getText();
>          }
>          mAsserter.is(urlbarText, url, "Awesomebar URL stayed the same");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);

I have been uncomfortable with the behavior of verifyUrl, but changing it affects a lot of tests, including some that are currently disabled. Are you sure you want to do this?

@@ +844,5 @@
> +            }
> +        }
> +
> +        // Solo doesn't close the menu when an item is clicked, so do it
> +        // manually

I find this comment confusing. Solo does not close the menu, but usually selecting a menu item will close the menu. What exactly are you guarding against, and why? 

One possibility that I see is that the menu is opened, the expected item is not found, so nothing is clicked, and then the test continues with the menu open. Your change would help ensure that the menu is closed when the test continues. That might improve the chances of the test passing -- even though an unexpected condition was encountered. Is that what we want?

Comment 13

[Brian Nicholson (:bnicholson)]

(In reply to Geoff Brown [:gbrown] from comment #12)
> Comment on attachment 756865 [details] [diff] [review]
> Part 1: Robustify existing test methods
> 
> Review of attachment 756865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure that changes are required here for an r+, but let's discuss
> these points and then re-visit the review.
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +275,4 @@
> >              urlbarText = urlbar.getText();
> >          }
> >          mAsserter.is(urlbarText, url, "Awesomebar URL stayed the same");
> > +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> 
> I have been uncomfortable with the behavior of verifyUrl, but changing it
> affects a lot of tests, including some that are currently disabled. Are you
> sure you want to do this?

It's used by 7 tests, but in 5 of those 7 tests, it's the last method called, so I don't think adding a back press should have any effect on those. I felt the API was cleaner this way, but you spend a lot more time looking at tests than I do, so I'll let you make the call :)

> @@ +844,5 @@
> > +            }
> > +        }
> > +
> > +        // Solo doesn't close the menu when an item is clicked, so do it
> > +        // manually
> 
> I find this comment confusing. Solo does not close the menu, but usually
> selecting a menu item will close the menu. What exactly are you guarding
> against, and why? 

Perhaps it's device-specific, but at least on my Droid RAZR, I found that the menu remained visible even after an item was clicked. I think it was actually clicked (and not just not found) since the browser actually went forward in the session history. This broke cases that did forward navigation followed by back (since the menu was still open and consumed the back press).

Comment 14

[Geoff Brown [:gbrown]]

Comment on attachment 756869 [details] [diff] [review]
Part 3: Session restore tests

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

This did get complicated. You might have been better off splitting some ideas into a separate bug, or just keeping it simple. Still, this looks good and I appreciate all the effort.

There are a couple of ideas below that do not require action. r+ withheld only for a successful try run.

::: mobile/android/base/tests/BaseTest.java.in
@@ +965,5 @@
> +     *
> +     * This can be used in methods called via waitForTest() where an assertion
> +     * might not immediately succeed.
> +     */
> +    public class NonFatalAsserter extends FennecMochitestAssert {

I am a little concerned that this might be abused or cause confusion in the future. I would use a regular mAsserter.dumplog to dump diagnostics in the moment, and then return status from the waitForTest test function (and/or its callees) rather than checking a NonFatalAsserter.ok. Just a thought -- I do not strongly object.

::: mobile/android/base/tests/testSessionRestoreOOMSave.java.in
@@ +10,5 @@
> + *
> + * Builds a session and tests that the saved state is correct.
> + */
> +public class testSessionRestoreOOMSave extends SessionTest {
> +    private final static int SESSION_TIMEOUT = 15000;

If the typical case is about 9000 ms (comment below), I would make the max SESSION_TIMEOUT at least 25000 ms, unless you feel there is a good reason for a lower upper bound. We want these tests to run reliably on a wide range of devices and we need very high reliability on tbpl.

Comment 15

[Geoff Brown [:gbrown]]

(In reply to Brian Nicholson (:bnicholson) from comment #13)
> (In reply to Geoff Brown [:gbrown] from comment #12)
> > > +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> > 
> > I have been uncomfortable with the behavior of verifyUrl, but changing it
> > affects a lot of tests, including some that are currently disabled. Are you
> > sure you want to do this?
> 
> It's used by 7 tests, but in 5 of those 7 tests, it's the last method
> called, so I don't think adding a back press should have any effect on
> those. I felt the API was cleaner this way, but you spend a lot more time
> looking at tests than I do, so I'll let you make the call :)

I recommend not making this change.
 
> > @@ +844,5 @@
> > > +        // Solo doesn't close the menu when an item is clicked, so do it
> > > +        // manually
> > 
> > I find this comment confusing. Solo does not close the menu, but usually
> > selecting a menu item will close the menu. What exactly are you guarding
> > against, and why? 
> 
> Perhaps it's device-specific, but at least on my Droid RAZR, I found that
> the menu remained visible even after an item was clicked. I think it was
> actually clicked (and not just not found) since the browser actually went
> forward in the session history. This broke cases that did forward navigation
> followed by back (since the menu was still open and consumed the back press).

OK. Consider changing the comment to something like "On some devices, the menu may not be dismissed after clicking on an item. Close it here."

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Robustify existing test methods, v2

Addressed comments.

Comment 17

[Brian Nicholson (:bnicholson)]

Sadly, I'm seeing a lot of failures when running many instances of this test: https://tbpl.mozilla.org/?tree=Try&rev=f5aac3003174. Seems as if the profile directory is wrong.

Filed bug 880599.

Comment 18

[Geoff Brown [:gbrown]]

Comment on attachment 759518 [details] [diff] [review]
Part 1: Robustify existing test methods, v2

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

Thanks.

Comment 19

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Session restore tests, v2

I rebased this on top of fig and weeded out the few remanining intermittents. My last few try pushes a month ago had some intermittent ARMv6 failures I couldn't pin down (hence my verbose logging in the try push below), but those failures are gone now; my suspicion is that they were fixed by bug 838210.

I tried your suggestion about using dumpStack() in place of a custom asserter, but that resulted in many superfluous stack traces being dumped to the log while we waited for the session file to update. Also, when there was a failure, it took more effort to track down the relevant stack trace in the log since it wasn't the actual place where the test failed. As a result, I kept the NonFatalAsserter. To address your concern about this being abused elsewhere, I moved the NonFatalAsserter into SessionTest -- let me know if this works for you.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=19ef54112198

The lone failure there looks like an unrelated failure that appeared after the Solo upgrade, and the exception is already filed as bug 911105.

Comment 20

[Brian Nicholson (:bnicholson)]

Filed bug 917833 for the TODO comment in testSessionOOMRestore.java.in.

Comment 21

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #19)
> The lone failure there looks like an unrelated failure that appeared after
> the Solo upgrade, and the exception is already filed as bug 911105.

Hm, bug 911105 actually appeared before the Robotium upgrade, so something else must have triggered that failure.

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 806653 [details] [diff] [review]
Part 3: Session restore tests, v2

I think we'll need a boolean to keep track of whether Session:Restore was called (see https://bugzilla.mozilla.org/show_bug.cgi?id=917833#c2), so removing r? until the patch is updated.

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Robustify existing test methods, v3. r=gbrown

Rebased.

Comment 24

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Add stateBundle extra to allow injecting fake state bundles, v2. r=mfinkle,gbrown

Rebased.

Comment 25

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Session restore tests, v3

Since these have been sitting awhile, I rebased them and did a try run to see if the intermittents were fixed. Results are encouraging: https://tbpl.mozilla.org/?tree=Try&rev=33efa8656d2a

After pushing to try, I realized the tests still used the old waitForTest() instead of waitForCondition(). I updated this patch and verified that tests still pass locally, but I didn't think it was worth another full try run for the change.

Comment 26

[Geoff Brown [:gbrown]]

Comment on attachment 831809 [details] [diff] [review]
Part 3: Session restore tests, v3

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

Good to see this revived!

Your try run looks good, but remember that these patches also affect common code (BaseTest), so you might want to double-check with a run through all the robocop tests.

::: mobile/android/base/tests/robocop.ini
@@ +64,5 @@
>  # [testTabHistory] # see bug 915350
>  # [testThumbnails] # see bug 813107
>  # [testVkbOverlap] # see bug 907274
> +[testSessionOOMSave]
> +[testSessionOOMRestore]

We are keeping robocop.ini alphabetized now.

Comment 27

[Brian Nicholson (:bnicholson)]

(In reply to Geoff Brown [:gbrown] from comment #26)
> Your try run looks good, but remember that these patches also affect common
> code (BaseTest), so you might want to double-check with a run through all
> the robocop tests.

General try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=1da4e55b5b65

Comment 28

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/43549d3c3333
https://hg.mozilla.org/integration/fx-team/rev/13dbefc311ef
https://hg.mozilla.org/integration/fx-team/rev/8810e56aa806

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/43549d3c3333
https://hg.mozilla.org/mozilla-central/rev/13dbefc311ef
https://hg.mozilla.org/mozilla-central/rev/8810e56aa806