Closed Bug 1260478 Opened 4 years ago Closed 3 years ago

Move TabsProvider integration tests to unit tests

Categories

(Firefox for Android :: Testing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: nalexander, Assigned: mcomella)

References

Details

Attachments

(4 files)

This ticket tracks moving some of the following tests from integration tests (junit3 and/or Robocop) to unit tests (junit4 with robolectric):

mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestClientsDatabase.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestClientsDatabaseAccessor.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestFennecTabsRepositorySession.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/TestFennecTabsStorage.java
mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/sync/TestTabsRecord.java
mobile/android/tests/browser/junit3/src/org/mozilla/tests/browser/junit3/TestRemoteTabs.java
(In reply to Nick Alexander :nalexander from comment #0)
> This ticket tracks moving some of the following tests from integration tests
> (junit3 and/or Robocop) to unit tests (junit4 with robolectric):
> 
> mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/
> TestClientsDatabase.java
> mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/
> TestClientsDatabaseAccessor.java

grisha will be changing these significantly in Bug 1046709.

> mobile/android/tests/background/junit3/src/org/mozilla/gecko/background/db/
> TestFennecTabsRepositorySession.java

This is testing Sync and not really the DB layer.  It can and should move, but doesn't need to in this ticket.
Notes:

* Setting the package name in robolectric.properties lets us read
  resources.  If we don't, Robolectric tries to read from
  org.mozilla.fennec_$USER or similar.

* We need DelegatingTestContentProvider not for isolation but to
  append "test=1" to all URIs.  Robolectric provides isolation by
  starting each test in a clean environment, but if we don't tell the
  CP to run in test mode, it tries to write into DBs that Robolectric
  doesn't like.

* Robolectric needs manual "shimming", i.e. the test must tell the
  ShadowContentResolver how to resolve.  We also need to handle
  shutdown() ourselves.  Basically, Robolectric doesn't try to
  duplicate the entire Android ContentProvider lifecycle.

* We might grow a "ContentProviderTest" base class to handle the
  registration and shutdown in the future.  I find such base classes
  frustrating and limiting in our Robocop tests, so I'd like to try to
  avoid them in our unit tests for as long as possible.

Review commit: https://reviewboard.mozilla.org/r/42993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42993/
Attachment #8735934 - Flags: review?(michael.l.comella)
Attachment #8735935 - Flags: review?(michael.l.comella)
Attachment #8735936 - Flags: review?(michael.l.comella)
Attachment #8735937 - Flags: review?(michael.l.comella)
None of these were run in automation anyway.  I elected to hg rm,
rather than try to hg mv, since I reworked the tests a little for
Robolectric, including merging two into one.  The history isn't
particularly valuable here.

Review commit: https://reviewboard.mozilla.org/r/42999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42999/
Assignee: nobody → nalexander
Comment on attachment 8735934 [details]
MozReview Request: Bug 1260478 - Part 1: Duplicate TestFennecTabsStorage integration test into TestTabsProvider unit test. r?mcomella

https://reviewboard.mozilla.org/r/42993/#review40887

Only skimmed the actual tests because this is a port.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/DelegatingTestContentProvider.java:1
(Diff revision 1)
> +package org.mozilla.gecko.background.db;

nit: license

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/DelegatingTestContentProvider.java:33
(Diff revision 1)
> +    }
> +
> +    private Uri appendTestParam(Uri uri) {
> +        try {
> +            return appendUriParam(uri, BrowserContract.PARAM_IS_TEST, "1");
> +        } catch (Exception e) {

I'd think we'd want to throw if we can't append here.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java:63
(Diff revision 1)
> +        return cr.acquireContentProviderClient(BrowserContractHelpers.TABS_CONTENT_URI);
> +    }
> +
> +    protected int deleteTestClient(final ContentProviderClient clientsClient) throws RemoteException {
> +        if (clientsClient == null) {
> +            return -1;

Why not throw?

Same below.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java:72
(Diff revision 1)
> +
> +    protected int deleteAllTestTabs(final ContentProviderClient tabsClient) throws RemoteException {
> +        if (tabsClient == null) {
> +            return -1;
> +        }
> +        return tabsClient.delete(BrowserContractHelpers.TABS_CONTENT_URI, TABS_CLIENT_GUID_IS, new String[]{TEST_CLIENT_GUID});

nit: spacing around `{...}`

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java:117
(Diff revision 1)
> +        Assert.assertNotNull(tabsClient);
> +        tabsClient.release();
> +    }
> +
> +    @Test
> +    public void testWipeClients() throws RemoteException {

nit: `testDeleteEmpty*` might be more accurate. Same below

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java:191
(Diff revision 1)
> +            Assert.assertFalse(cursor.moveToNext());
> +        } finally {
> +            cursor.close();
> +        }
> +
> +        int deleted = clientsClient.delete(uri, null, null);

I know this is a port so I don't expect you to fix this, but we should test deletion of actual rows (rather than deleting empty) before we use it in another test.
Attachment #8735934 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8735935 [details]
MozReview Request: Bug 1260478 - Part 2: Duplicate TestTabsRecord into TestTabsProvider unit test. r?mcomella

https://reviewboard.mozilla.org/r/42995/#review40897

Only skimmed since this is a port.
Attachment #8735935 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8735936 [details]
MozReview Request: Bug 1260478 - Part 3: Duplicate TestRemoteTabs integration test into unit tests. r?mcomella

https://reviewboard.mozilla.org/r/42997/#review40899

Again, only skimmed.
Attachment #8735936 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8735937 [details]
MozReview Request: Bug 1260478 - Post: Remove TabsProvider integration tests moved into unit tests. r?mcomella

https://reviewboard.mozilla.org/r/42999/#review40901
Attachment #8735937 - Flags: review?(michael.l.comella) → review+
mcomella: in the spirit of delegating, can you drive this to landing?  I think it's valuable enough to start transitioning to off-device testing for ContentProvider.

Perhaps a mentor ticket to address the nits?
Flags: needinfo?(michael.l.comella)
https://reviewboard.mozilla.org/r/42993/#review40887

> I'd think we'd want to throw if we can't append here.

I removed the catch `Exception` altogether – the contained code doesn't actually throw checked Exceptions. I'm not sure if there was more intended to be caught here though.
Taking over for Nick and assigning myself so I'm responsible for this bug.

Unfortunately, I didn't think to do a mentor ticket to address the nits and I just saw comment 11 so I did it myself.
Assignee: nalexander → michael.l.comella
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/957a0ef217ba730f3ebd795193775b7a81687c35
Bug 1260478 - Part 1: Duplicate TestFennecTabsStorage integration test into TestTabsProvider unit test. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/71800b3bf85c72d9b1b4ff84231248c0da0ad52c
Bug 1260478 - Part 2: Duplicate TestTabsRecord into TestTabsProvider unit test. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/25e433a0aed40d1d2cd4f0680acc9703c81beae5
Bug 1260478 - Part 3: Duplicate TestRemoteTabs integration test into unit tests. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/534d3392d794f3d885348bc0998b9751027012c7
Bug 1260478 - Post: Remove TabsProvider integration tests moved into unit tests. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/90a025addc58d0bbbbcde962f209dfea1dde81da
Bug 1260478 - Fixed review nits from previous commits. r=me
You need to log in before you can comment on or make changes to this bug.