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/

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

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

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/

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.

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.

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.

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

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?

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.

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

https://hg.mozilla.org/mozilla-central/rev/957a0ef217ba https://hg.mozilla.org/mozilla-central/rev/71800b3bf85c https://hg.mozilla.org/mozilla-central/rev/25e433a0aed4 https://hg.mozilla.org/mozilla-central/rev/534d3392d794 https://hg.mozilla.org/mozilla-central/rev/90a025addc58