Closed Bug 993970 Opened 6 years ago Closed 5 years ago

Implement tests for TopSitesCursorWrapper

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(4 files)

Prep work for suggested sites. I want to be confident that my changes to the cursor wrapper will not break anything.
Comment on attachment 8403879 [details] [diff] [review]
Factor out TopSitesCursorWrapper into separate file (r=wesj)

(No functional changes)
Attachment #8403879 - Flags: review?(wjohnston)
Attachment #8403880 - Flags: review?(wjohnston)
Comment on attachment 8403881 [details] [diff] [review]
Coding style fixes in TopSitesCursorWrapper (r=wesj)

(No functional changes)
Attachment #8403881 - Flags: review?(wjohnston)
Comment on attachment 8403882 [details] [diff] [review]
Add tests for TopSitesCursorWrapper (r=wesj)

Tests the cursor wrapper behaviour as implemented today.
Attachment #8403882 - Flags: review?(wjohnston)
Attachment #8403879 - Flags: review?(wjohnston) → review+
Attachment #8403880 - Flags: review?(wjohnston) → review+
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?
Attachment #8403881 - Flags: review?(wjohnston) → review+
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 :)).
Attachment #8403882 - Flags: review?(wjohnston) → review+
(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.
(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.
Blocks: 996657
You need to log in before you can comment on or make changes to this bug.