Closed Bug 1235599 Opened 6 years ago Closed 6 years ago

Add tests for LocalURLMetadata

Categories

(Firefox for Android Graveyard :: Testing, defect)

defect
Not set
normal

Tracking

(firefox46 affected, firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox46 --- affected
firefox50 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

Attachments

(2 files)

We currently have no tests for the site metadata storage (preview tiles, tile colors, and touch icons as of Bug 826400) - we should add tests for this.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java

This is especially important because of the caching at:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java#114
Assignee: nobody → ahunt
Depends on: 826400
Status: NEW → ASSIGNED
Comment on attachment 8760409 [details]
Bug 1235599 - Make UrlMetadataTable.CONTENT_URI public to allow using in tests

https://reviewboard.mozilla.org/r/57976/#review54930
Attachment #8760409 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8760410 [details]
Bug 1235599 - Add basic UrlMetadata tests

https://reviewboard.mozilla.org/r/57978/#review55544

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1523
(Diff revision 1)
> +    private class TestInsertUrlMetadata extends TestCase {
> +        @Override
> +        public void test() throws Exception {
> +            testInsertionViaContentProvider();
> +            testInsertionViaUrlMetadata();
> +            testRetrievalViaUrlMetadata();

You should add a comment in here somewhere that this test relies on the results of the previous tests.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1543
(Diff revision 1)
> +                    new String[] {url1}
> +            );
> +
> +            final Cursor c = getUrlMetadataByUrl(url1);
> +            try {
> +                mAsserter.is(c.moveToFirst(), true, "URL metadata inserted via Content Provider not found");

It'd more explicit to check `Cursor.getCount`

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1564
(Diff revision 1)
> +
> +            getTestProfile().getDB().getURLMetadata().save(mResolver, data);
> +
> +            final Cursor c = getUrlMetadataByUrl(url2);
> +            try {
> +                mAsserter.is(c.moveToFirst(), true, "URL metadata inserted via UrlMetadata not found");

Same.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1571
(Diff revision 1)
> +            // LocalURLMetadata has some caching of results: we need to test that this caching
> +            // doesn't prevent us from accessing data that might not have been loaded into the cache.

nit: How do we do that? And how do we test around the cache? Are we trying to? Elaborate.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1601
(Diff revision 1)
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TILE_COLOR_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +
> +
> +            // 3: retrieve all columns for both URLs
> +            results = metadata.getForURLs(mResolver,
> +                    Arrays.asList(new String[] {

`Arrays.asList(url1, url2)` might just work, I'm not sure. I've seen sebastian use syntax like that before.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:1611
(Diff revision 1)
> +            mAsserter.is(results.containsKey(url1), true, "URL 1 not found in results");
> +            mAsserter.is(results.containsKey(url2), true, "URL 2 not found in results");
> +
> +            urlData = results.get(url1);
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TILE_IMAGE_URL_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TILE_COLOR_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TOUCH_ICON_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +
> +            urlData = results.get(url2);
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TILE_IMAGE_URL_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TILE_COLOR_COLUMN), true, "touchIcon column missing in UrlMetadata results");
> +            mAsserter.is(urlData.containsKey(URLMetadataTable.TOUCH_ICON_COLUMN), true, "touchIcon column missing in UrlMetadata results");

nit: you could do this in a loop

`for (final String url : new String[] { url1, url2 }) {`

Or re-using the list you made above.
Attachment #8760410 - Flags: review?(michael.l.comella) → review+
https://hg.mozilla.org/mozilla-central/rev/98220ccf6c93
https://hg.mozilla.org/mozilla-central/rev/bc55f3804d80
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.