Add tests for Profile Migration

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

Trunk
Firefox 16
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Profile Migration has zero tests right now. We should add some.
(Assignee)

Comment 1

6 years ago
Created attachment 626045 [details] [diff] [review]
WIP. Migration tests
Assignee: nobody → gpascutto
(Assignee)

Updated

6 years ago
Depends on: 735461
(Assignee)

Comment 2

6 years ago
Created attachment 626475 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

This works with the constraints we have right now, i.e. that we can't specify the profile we want to use for a test correctly, and that there's no real easy way to dump files into the profile.

Because of the above limitation, the order of tests is important (it already was, but these tests make it worse). I fixed one of the previous tests to clean up after itself, to make the assertions in this one a bit simpler.

Putting jmaher and geoffbrown in feedback? to see if there's anything in here that could've been done simpler.
Attachment #626045 - Attachment is obsolete: true
Attachment #626475 - Flags: review?(lucasr.at.mozilla)
Attachment #626475 - Flags: feedback?(jmaher)
Attachment #626475 - Flags: feedback?(gbrown)
(Assignee)

Comment 3

6 years ago
The places.db is a profile specifically made for this test, with multiple levels of bookmarks etc.

I wanted to add testing if Sync Migration works, but AFAIK there's no sure-fire way to know if it worked. Specifically we can't check if the setup succeeded because the device might have an account that comes from another installation (perhaps not a problem on try devices, but makes this test useless for developing).
(In reply to Gian-Carlo Pascutto (:gcp) from comment #3)
> The places.db is a profile specifically made for this test, with multiple
> levels of bookmarks etc.
> 
> I wanted to add testing if Sync Migration works, but AFAIK there's no
> sure-fire way to know if it worked. Specifically we can't check if the setup
> succeeded because the device might have an account that comes from another
> installation (perhaps not a problem on try devices, but makes this test
> useless for developing).

We have tests for account creation. Might be able to steal something here, depending on what you want to do. But not super urgent IMO.
Comment on attachment 626475 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

> Putting jmaher and geoffbrown in feedback? to see if there's anything in here
> that could've been done simpler.

Sorry, no silver bullets here. Until/unless bug 735461 is addressed, it's going to be difficult to test scenarios like this. Perhaps you could add a comment to robotium.ini:

# This test must run before ... -- see bugs 750753 and 735461

(And I will try to look at 735461 again next week.)
Attachment #626475 - Flags: feedback?(gbrown) → feedback+
Comment on attachment 626475 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

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

overall this looks good.

::: mobile/android/base/tests/testMigration.java.in
@@ +234,5 @@
> +        }
> +
> +        // Just return null if we can't find the bookmarks list view
> +        return null;
> +    }

don't we have other test cases that open the bookmarks?
Attachment #626475 - Flags: feedback?(jmaher) → feedback+
Comment on attachment 626475 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

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

::: mobile/android/base/tests/testMigration.java.in
@@ +99,5 @@
> +            return;
> +        }
> +
> +        // Reset history entries from previous tests
> +        clearHistory();

It bugs me a bit that this test is not isolated from the others. I wrote testBrowserProvider in such a way that it works with a temporary database unique to the test. Isn't that possible with the migration stuff?

@@ +190,5 @@
> +            }
> +        }
> +
> +        // Open the bookmark list and check the root folder view
> +        ListView bookmarksList = openBookmarksList();

Hmm, I kind of expected this test to be UI-less as you're dealing with data-only APIs here. Why do you need to get the list from the UI?
(Assignee)

Comment 8

6 years ago
>It bugs me a bit that this test is not isolated from the others. I wrote 
>testBrowserProvider in such a way that it works with a temporary database unique 
>to the test. Isn't that possible with the migration stuff?

See the "Depends on" bug 735461. The "temporary database" stuff currently doesn't work.

>Hmm, I kind of expected this test to be UI-less as you're dealing with data-only 
>APIs here. Why do you need to get the list from the UI?

It's the same test as in testBookmark to see if the added bookmarks are actually showing up. Could change this to a UI-less test if you prefer.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #8)
> >It bugs me a bit that this test is not isolated from the others. I wrote 
> >testBrowserProvider in such a way that it works with a temporary database unique 
> >to the test. Isn't that possible with the migration stuff?
> 
> See the "Depends on" bug 735461. The "temporary database" stuff currently
> doesn't work.

testBrowserProvider doesn't set a custom profile. It simply names the db for a given profile differently so that it doesn't affect (and it's not affected by) other tests. See ContentProviderTest.

> >Hmm, I kind of expected this test to be UI-less as you're dealing with data-only 
> >APIs here. Why do you need to get the list from the UI?
> 
> It's the same test as in testBookmark to see if the added bookmarks are
> actually showing up. Could change this to a UI-less test if you prefer.

I generally try to avoid involving too many components in the same tests. So, I'd prefer this test to be UI-less.
(Assignee)

Comment 10

6 years ago
>It bugs me a bit that this test is not isolated from the others. I wrote 
>testBrowserProvider in such a way that it works with a temporary database unique 
>to the test. Isn't that possible with the migration stuff?

Everything is possible, but...

The BrowserProvider isolation works by appending a query parameter to all requests and then snatching this parameter when finding the database name. The current tests use Profile Migration (which has nothing to do with ContentResolver/ContentProvider API) and BrowserDB (which doesn't have a similar API either).

Given that I have other patches pending that move more Migration to BrowserDB, the only way to get this to work is to extend both BrowserDB and Profile Migration to take a similar "test" mode parameter.

All of this while it's know to be a bug that profiles aren't separated in tests to begin with.
(Assignee)

Comment 11

6 years ago
Created attachment 632743 [details] [diff] [review]
Patch 2. Isolate migration tests

This isolates the Migration testing by extending the API and removing the UI part of the tests as discussed in the previous comments.
Attachment #632743 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 632743 [details] [diff] [review]
Patch 2. Isolate migration tests

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

Maybe I should have been more explicit about what I meant with "isolating the test". Just using the PARAM_TEST query argument in the provider will not ensure that tests are isolated. It will only avoid using a full path db file in the SqliteOpenHelper to allow the context define where to save the new db file. If two tests just use PARAM_TEST like you did here, they will affect each other. See:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserProvider.java.in#1067

I only needed to add PARAM_TEST due to a bug in RenamingDelegatingContext that doesn't support renaming absolute paths correctly. See how the content provider is setup in ContentProviderTest:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/ContentProviderTest.java.in#135

The idea here is to isolate the BrowserProvider tests in a transparent way, with no changes to, say, the BrowserDB API. It will only use a special ContentResolver that uses an isolated runtime environment under the hood. See the use of IsolatedContext here:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/ContentProviderTest.java.in#145

See, for example, how I managed to use BrowserDB API as is in an isolated context in testBrowserProviderPerf:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBrowserProviderPerf.java.in

So, ideally speaking (not sure if it's entirely possible), you should be able to inherit testMigration from ContentProviderTest.
(Assignee)

Comment 13

6 years ago
Aside from the issues Lucas discussed, I have the more simple problem that in the current version, I can get the assets/getAssets functions to work to extract the places.sqlite file into the profile dir, but only on my own device.

If I try the test on the tryserver, it fails with an IOException when trying to read places.sqlite:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12752862&tree=Try&full=1#error0

I fudged the test further to show the assets that are detected by the AssetManager:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12760564&tree=Try&full=1#error0
[README, images, places.sqlite, sounds, testcheck2-motionevents, webkit]

So the places.sqlite file is in the expected location, but trying to read it with getAssets from BaseTest fails. But only on tryserver, it works on the local device.

Help?
(Assignee)

Comment 14

6 years ago
>If I try the test on the tryserver, it fails with an IOException when trying to 
>read places.sqlite:

Should've linked to this one to see the original Exception:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12754326&tree=Try&full=1#error0
(Assignee)

Comment 15

6 years ago
Found the problem: Android can't deal with assets greater than 1M because it decompresses them into RAM. Well, some Androids can't - it seems the firmware on the SGS2 has a greater tolerance but the one on the try Tegras doesn't.

I fixed it by zipping the database, which prevents the APK from packing it, and then manually unzipping into temp storage during the test.

I'll look into using the ContentProviderTest wrapper again now, I have a partial patch that uses it, but I need to wrap some more Context methods that Profile Migration needs before it can work that way.
(Assignee)

Comment 16

6 years ago
Created attachment 636697 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

I believe this is unchanged from before but resubmitting just to be sure.
Attachment #626475 - Attachment is obsolete: true
Attachment #626475 - Flags: review?(lucasr.at.mozilla)
Attachment #636697 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 17

6 years ago
Created attachment 636698 [details] [diff] [review]
Patch 2. Isolate migration tests

This does what is necessary for this to work by wrapping with the DelegatingContextProvider that adds the IS_TEST flag transparently:

- It extends BaseTest so the Activity is started, this is needed to access the assets.
- It works around (compressed) assets not being allowed to be larger than 1M by zipping them and unzipping into the profile.
- It switches getInstrumentation/getContext in favor in getActivity in one case where it's needed to correctly find the resources.
- It wraps some additional Context methods needed for Profile Migration to work.

It also addresses some remarks from the earlier reviews.
Attachment #636698 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Updated

6 years ago
Attachment #632743 - Attachment is obsolete: true
Attachment #632743 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 18

6 years ago
Created attachment 636749 [details] [diff] [review]
Patch 1. Add tests for Profile Migration

Attaching the right patch would help. Note that some nits here are corrected in the next patch.
Attachment #636697 - Attachment is obsolete: true
Attachment #636697 - Flags: review?(lucasr.at.mozilla)
Attachment #636749 - Flags: review?(lucasr.at.mozilla)

Updated

6 years ago
Attachment #636749 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 636698 [details] [diff] [review]
Patch 2. Isolate migration tests

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

There's a non-obvious mix of activity context and isolated context in some parts of ContentProviderTest. Please add a "big picture" comment at the top ContentProviderTest explaining why we have to do this. IIUC, I didn't inherit from BaseTest before because it would bring a lot of UI stuff with it. Does inheriting from BaseTest cause any subtle behaviour change in the content provider test environment? If so, add that to the comment I suggested.
Attachment #636698 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a847f96dec34
https://hg.mozilla.org/mozilla-central/rev/d0ac157afcb7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.