1351389 - Write additional tests for TestTabsProvider

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Android Sync, enhancement, P5)

Description

[Edouard Oger [:eoger]]

We resolved some Open Tabs syncing issues with bug 1265314, namely:
- Closing all tabs now leads to the upload of an updated tabs record.
- Opening a tab in the background also leads to the upload of an updated tabs record.

We fixed these problems by changing the way we assign the tab record lastModified field.
We set it to the local client last_modified attribute, which is bumped every time a tab is opened, updated or closed (see LocalTabsAccessor.java#persistLocalTabs and FennecTabsRepository.java#getLocalClientLastModified).

We need to extend our test coverage in TestTabsProvider to cover these new cases and prevent regressions.
Here are some tests that should be written:
* insert two or three regular tabs, client time is >= max
* insert regular, than last_used = 0 tabs, client time is >= regular
* insert last_used = 0 (ie. background) tabs, client time is "reasonable"

Comment 1

[mddrill]

Hi, I would like to work on this. I have a few questions though. 

I'm assuming when you write "insert two or three regular tabs, client time is >= max", that means write a test that inserts two or three regular tabs then assert that a variable client_time >= a variable max. Is this correct? 

Also, what do you mean by "insert regular, than last_used = 0 tabs". What does inserting regular mean, and what does "than last_used = 0 tabs" mean? and what do you mean by "client time is 'reasonable'". What is reasonable?

Comment 2

[Edouard Oger [:eoger]]

Hello there,

The suggested test cases are copy pasted from nalexander in bug 1265314 comment 18. Here are some clarifications:

- First test: Insert 2 or 3 "regular" tabs, then assert that when we create the client record, clientRecord.last_modified is >= MAX(tabs.lastUsed)

- What's a "regular" tab? A tab which lastUsed property is > 0 (eg. a timestamp from today).

- "than last_used = 0 tabs". This is probably a typo. He meant "insert tabs with a lastUsed property set to 0" (which is what happens for background tabs).

- By reasonable I'm guessing he means > 0. You could probably use the main process start timestamp.

Thank you for trying to tackle this, this is not a trivial good-first-bug (this should probably be a good-second-bug actually), don't hesitate to ask for more help when you're blocked.

Comment 3

[mddrill]

I know this isn't exactly trivial, but I think I can handle it if you give me some help. 

So, should I create a new test method for this or insert this stuff into one of the existing methods? (I'm guessing the former, but just making sure). If the former, should I create a new method for each of these and what should I call the methods? (I don't know what name would make sense, and I don't want to just put something like "testingTabs()"

Comment 4

[mddrill]

Another question. TestTabsProvider.java seems to import Classes from several modules in `org` I'll copy and paste it so you can see what I'm talking about

import org.json.simple.JSONArray;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mozilla.gecko.background.testhelpers.TestRunner;
import org.mozilla.gecko.db.BrowserContract;
import org.mozilla.gecko.db.TabsProvider;
import org.mozilla.gecko.sync.repositories.android.BrowserContractHelpers;
import org.mozilla.gecko.sync.repositories.android.FennecTabsRepository;
import org.mozilla.gecko.sync.repositories.domain.TabsRecord;
import org.robolectric.shadows.ShadowContentResolver;

But there seems to be at least 9 different directories in mozilla-central called org, how do I know which one this file is getting these modules from? (I'm not even sure if module is the right word, but you know what I mean).

Comment 5

[mddrill]

That may have been a stupid question, I'm guessing it's the org directory that is the ancestor of the directory TestTabsProvider is from.

Comment 6

[mddrill]

And I was wrong about that, so now I'm confused. I'll stop polluting this thread with comments now.

Comment 7

[Edouard Oger [:eoger]]

- This test probability belongs in TestFennecTabsRepositorySession, I would create a dedicated test method for that.

- Indeed java packages can come from multiple places in the tree, I'm not sure what your question is?

Nick should probably know a bit more than me on how to implement these tests.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Edouard Oger [:eoger] from comment #7)
> - This test probability belongs in TestFennecTabsRepositorySession, I would
> create a dedicated test method for that.
> 
> - Indeed java packages can come from multiple places in the tree, I'm not
> sure what your question is?
> 
> Nick should probably know a bit more than me on how to implement these tests.

There are a bunch of examples of pulling tab record out the content provider in 

https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java

and

https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProviderRemoteTabs.java.

This test has to do with local tabs (not remote tabs), so add tests to TestTabsProvider: copy a test function, change the name,  and modify the insertions and fetches to test the scenarios listed above.

These are JUnit 4 tests: they run on your laptop, not on the Android device.  You can run the tests usually by right-clicking the test file and running as a test.

Comment 9

[mddrill]

Is there a way to get the clientRecord by querying the tabsClient? Or do I need to create a clientRecord object like on line 274 of https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java ?

Also, when I query the tabs client to get information about the tabs, what method can I use to get an array of the ints in the last_modified column of the database?

And to insert tabs where lastUsed is 0, I'm going to create a new method by copying and pasting insertSomeTestTabs, is that good or is there a better way to do it?

Just to make sure I'm doing this right, I'm going to:

1) insert 3 tabs where lastUsed is >0 (it seems like insertTestTabs already does this, so I will use that method), the assert that the lastModified property of the client record is >= the maximum of the lastUsed properties of all the tabs

2) insert 3 tabs with the lastUsed properties set to 0 (by copying and pasting insertSomeTestTabs like I specified above?), then assert that the lastModified property of the client record is >= (What am I asserting this is >= to?)

3) use the same 3 tabs from above, then assert that the lastModified property of the client record is > 0

Is this okay?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to mddrill from comment #9)
> Is there a way to get the clientRecord by querying the tabsClient? Or do I
> need to create a clientRecord object like on line 274 of
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/
> main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.
> java ?

Not really.  Follow the examples I linked to, just like at https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/db/TestTabsProvider.java#253.

> Also, when I query the tabs client to get information about the tabs, what
> method can I use to get an array of the ints in the last_modified column of
> the database?

Look around http://searchfox.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/db/Tab.java#82, which I think is already doing what you want.  (I don't see a last_modified column in the Java code.)

> And to insert tabs where lastUsed is 0, I'm going to create a new method by
> copying and pasting insertSomeTestTabs, is that good or is there a better
> way to do it?

Yes, that's fine.

> Just to make sure I'm doing this right, I'm going to:
> 
> 1) insert 3 tabs where lastUsed is >0 (it seems like insertTestTabs already
> does this, so I will use that method), the assert that the lastModified
> property of the client record is >= the maximum of the lastUsed properties
> of all the tabs
> 
> 2) insert 3 tabs with the lastUsed properties set to 0 (by copying and
> pasting insertSomeTestTabs like I specified above?), then assert that the
> lastModified property of the client record is >= (What am I asserting this
> is >= to?)
> 
> 3) use the same 3 tabs from above, then assert that the lastModified
> property of the client record is > 0

No.  The point is that writing tabs with different last_used times *also* writes the local client's modified time, as returned by http://searchfox.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java#186.

So you need tin insert tabs with the various parameters, and then verify that the local client's modified time is appropriate.  By "reasonable", I mean use System.currentTimeMillis() at the beginning of the test and assert that the modified time when you test is both after that time and not more than 30 * 1000 millis (30s) after that time.

Comment 11

[Min Yu]

Hi, I'm trying to write the unit testing for the first test case. I'm having trouble getting the local clients.lastModified. Following the examples linked above, the test examples set the last modified to a specific 1234. I was able to pull the lastUsed data from |tabsRecord.get(0 || 1 || 2).lastUsed|.

Comment 12

[Edouard Oger [:eoger]]

Have a look here:
http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/FennecTabsRepository.java#180

Comment 13

[Min Yu]

Looking at the function getLocalClientLastModified(), it's a private function so i assume we cannot just simply call the function.
Instead, I tried to initialize cursor like in the method to use RepoUtils.getLongFromCursor(cursor, Clients.LAST_MODIFIED);

In the function, cursor is set to query from tabsProvider.
>line# 185  cursor = clientsHelper.safeQuery(tabsProvider, ".fetchLocalClient()", null,
>                localClientSelection, localClientSelectionArgs, null);

both tabsProvider and clientsHelper are initialized using context. What exactly is context in the TestFennecTabsRepository?
>line# 84 ContentProviderClient  tabsProvider = getContentProvider(context, BrowserContractHelpers.TABS_CONTENT_URI);
>line# 95 RepoUtils.QueryHelper clientsHelper = new RepoUtils.QueryHelper(context, BrowserContractHelpers.CLIENTS_CONTENT_URI, 
>           LOG_TAG);

I am still having trouble initializing the variables to get Clients.LAST_MODIFIED.

Comment 14

[Min Yu]

Attachment: Bug 1351389 - First of three test cases

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

Comment 15

[Edouard Oger [:eoger]]

Sorry I don't have time to work on this anymore.

Comment 16

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.