993970 - Implement tests for TopSitesCursorWrapper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Lucas Rocha (:lucasr)]

Prep work for suggested sites. I want to be confident that my changes to the cursor wrapper will not break anything.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Factor out TopSitesCursorWrapper into separate file (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Make PinnedSite instances immutable (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Coding style fixes in TopSitesCursorWrapper (r=wesj)

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Add tests for TopSitesCursorWrapper (r=wesj)

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8403879 [details] [diff] [review]
Factor out TopSitesCursorWrapper into separate file (r=wesj)

(No functional changes)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8403881 [details] [diff] [review]
Coding style fixes in TopSitesCursorWrapper (r=wesj)

(No functional changes)

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8403882 [details] [diff] [review]
Add tests for TopSitesCursorWrapper (r=wesj)

Tests the cursor wrapper behaviour as implemented today.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8403881 [details] [diff] [review]
Coding style fixes in TopSitesCursorWrapper (r=wesj)

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

::: mobile/android/base/db/TopSitesCursorWrapper.java
@@ +19,4 @@
>          public final String title;
>          public final String url;
>  
> +        public PinnedSite(String title, String url) {

Any reason not to make the arguments final (here and throughout)?

@@ +25,5 @@
>          }
>      }
>  
> +    // The cursor for the top sites query
> +    private Cursor topSitesCursor;

And any reason not to make these two  cursor fields final (or the 'count' one?). I guess with these Cursors/Arrays its less helpful, since the contents can still be modified. Maybe we null them out when we're done as well?

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8403882 [details] [diff] [review]
Add tests for TopSitesCursorWrapper (r=wesj)

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

::: mobile/android/tests/browser/junit3/src/tests/TestTopSitesCursorWrapper.java
@@ +14,5 @@
> +import org.mozilla.gecko.db.BrowserContract.Bookmarks;
> +import org.mozilla.gecko.db.BrowserContract.Combined;
> +import org.mozilla.gecko.db.TopSitesCursorWrapper;
> +
> +public class TestTopSitesCursorWrapper extends BrowserTestCase {

Add a short summary of what's being tested here. Also, I can't find BrowserTestCase anywhere... what is it? I assume it does the work of iterating through all the 'test*' methods and calling them?

@@ +21,5 @@
> +                                                        Combined.URL,
> +                                                        Combined.TITLE,
> +                                                        Combined.DISPLAY,
> +                                                        Combined.BOOKMARK_ID,
> +                                                        Combined.HISTORY_ID };

I wish we had a better way to make sure these stayed in sync with the real cursors. i.e. If you change what's in these, I'd like to force you to update the test...

@@ +127,5 @@
> +        // But not the topsites cursor, of course.
> +        assertFalse(topSites.isClosed());
> +
> +        topSites.close();
> +        wrapper.close();

Hmm... We should probably just make wrapper.close close the topSites cursor (and test that here too :)).

Comment 10

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #8)
> Comment on attachment 8403881 [details] [diff] [review]
> Coding style fixes in TopSitesCursorWrapper (r=wesj)
> 
> Review of attachment 8403881 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/TopSitesCursorWrapper.java
> @@ +19,4 @@
> >          public final String title;
> >          public final String url;
> >  
> > +        public PinnedSite(String title, String url) {
> 
> Any reason not to make the arguments final (here and throughout)?

I'm not a fan of making method arguments final. It doesn't add a lot of value and adds verbosity. It's useful to express special intent of immutability in some contexts but this is not one of them.    

> @@ +25,5 @@
> >          }
> >      }
> >  
> > +    // The cursor for the top sites query
> > +    private Cursor topSitesCursor;
> 
> And any reason not to make these two  cursor fields final (or the 'count'
> one?). I guess with these Cursors/Arrays its less helpful, since the
> contents can still be modified. Maybe we null them out when we're done as
> well?

Good point. Done.

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Comment on attachment 8403882 [details] [diff] [review]
> Add tests for TopSitesCursorWrapper (r=wesj)
> 
> Review of attachment 8403882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/tests/browser/junit3/src/tests/TestTopSitesCursorWrapper.java
> @@ +14,5 @@
> > +import org.mozilla.gecko.db.BrowserContract.Bookmarks;
> > +import org.mozilla.gecko.db.BrowserContract.Combined;
> > +import org.mozilla.gecko.db.TopSitesCursorWrapper;
> > +
> > +public class TestTopSitesCursorWrapper extends BrowserTestCase {
> 
> Add a short summary of what's being tested here. Also, I can't find
> BrowserTestCase anywhere... what is it? I assume it does the work of
> iterating through all the 'test*' methods and calling them?

BrowserTestCase is the base class for browser junit3 tests. They can only be executed locally (at the moment) but are a lot lighter-weight than Robocop. See stuff in mobile/android/tests/browser/junit3 

The number of tests will grow quite a bit (in the upcoming tests). I'm trying to cover all the cursor features used in our UI. 

> @@ +21,5 @@
> > +                                                        Combined.URL,
> > +                                                        Combined.TITLE,
> > +                                                        Combined.DISPLAY,
> > +                                                        Combined.BOOKMARK_ID,
> > +                                                        Combined.HISTORY_ID };
> 
> I wish we had a better way to make sure these stayed in sync with the real
> cursors. i.e. If you change what's in these, I'd like to force you to update
> the test...

True. Can't think of a good way to enforce that though. Sharing the String[] array should be simple enough but we'd still need to update the tests to cover any new columns anyway.  
 
> @@ +127,5 @@
> > +        // But not the topsites cursor, of course.
> > +        assertFalse(topSites.isClosed());
> > +
> > +        topSites.close();
> > +        wrapper.close();
> 
> Hmm... We should probably just make wrapper.close close the topSites cursor
> (and test that here too :)).

Nice catch, updated the test accordingly.

Comment 12

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/1ceb041e811e
https://hg.mozilla.org/integration/fx-team/rev/2dea8051f145
https://hg.mozilla.org/integration/fx-team/rev/c31b455d6a36
https://hg.mozilla.org/integration/fx-team/rev/b207a9c58fb6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ceb041e811e
https://hg.mozilla.org/mozilla-central/rev/2dea8051f145
https://hg.mozilla.org/mozilla-central/rev/c31b455d6a36
https://hg.mozilla.org/mozilla-central/rev/b207a9c58fb6