907734 - Robocop: Split database interactions and strings definitions in separate classes to structure Robocop apis better

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch

This is a follow-up bug to all the work done on bug 901432. The AboutHomeTest class adds more structure to Robocop and makes BaseTest much more sensible in size, but from my work on re-writing testBookmark and testBookmarkPage I see AboutHomeTest getting crowded very fast. This patch moves all database interactions in a separate DatabaseHelper class and all major string declarations in the StringHelper class.

From different discussions with both Margaret and Lucas on the fig test re-writes it's clear that we are aiming to detach the tests from hard-coded strings as much as possible. The StringsHelper class is not complete and I haven't tackle making changes in all tests, but this, in my opinion, is a good start towards achieving as much independence from sting changes as possible.

I have attached a first attempt at a patch for this and I'm looking for some feedback on this idea.

Comment 1

[Geoff Brown [:gbrown]]

Comment on attachment 793491 [details] [diff] [review]
robocopRestructure.patch

I just had a quick look and support the general idea.

I noticed a bunch of Log.e() calls. Remember we have a strong preference for mAsserter.dumpLog/ok/is since we frequently do not report the full logcat in tbpl logs.

Comment 2

[Adrian Tamas (:AdrianT)]

I'll look into defining an asserter in the class

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 793491 [details] [diff] [review]
robocopRestructure.patch

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

Had a quick look. This is roughly in line with my thoughts on a new structure for our UI testing API which I'm starting to sketch out here: https://mobile.etherpad.mozilla.org/13. I'd like us to get a bit more disciplined on how to extend/expand the available APIs for tests. I don't have a concrete proposal to make just yet though. So I think it's fine to go ahead with this refactoring for now.

Comment 4

[Adrian Tamas (:AdrianT)]

I don't have access to the etherpad to look over what you have there. Can you please give me access to it if that's ok? I'll look into cleaning up this patch, adding the asserter as Geoff suggested and will post a follow-up patch when done.

Comment 5

[:Margaret Leibovic]

Comment on attachment 793491 [details] [diff] [review]
robocopRestructure.patch

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

Sorry I was so slow on the feedback, this is looking good.

::: mobile/android/base/tests/AboutHomeTest.java.in
@@ +23,3 @@
>      protected enum AboutHomeTabs {MOST_VISITED, MOST_RECENT, TABS_FROM_LAST_TIME, BOOKMARKS, READING_LIST};
>      protected ArrayList<String> aboutHomeTabs = new ArrayList<String>() {{
>                    add("HISTORY");

Should these also be updated to use StringHelper?

::: mobile/android/base/tests/DatabaseHelper.java.in
@@ +52,5 @@
> +        if (isBookmark(url)) {
> +            updateBookmark(title, url, null);
> +        } else {
> +            addMobileBookmark(title, url);
> +        }

Make sure to update this to include the change from bug 906221.

@@ +68,5 @@
> +            Log.e(LOGTAG, "Exception adding bookmark: " + e.toString());
> +        }
> +    }
> +
> +    protected void updateBookmark(String title, String url, String keyword) {

I should have looked at this patch sooner, I implemented something every similar for bug 908344. It looks like my patch there is a bit more succinct, and it will probably land first, so maybe we should just incorporate that into here.

@@ +106,5 @@
> +            Log.e(LOGTAG, "Exception deleting history item: " + e.toString());
> +        }
> +    }
> +
> +    protected Long getFolderId(String guid) {

Is there a reason you use Long here instead of long? Also, I see this very closely resembles LocalBrowserDB.getFolderIdFromGuid:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#534

I would prefer if we give it the same name, and add a comment saying it does the same thing. Maybe we could even file a separate bug to make that into a public method so that tests can use it.

::: mobile/android/base/tests/StringHelper.java.in
@@ +2,5 @@
> +package @ANDROID_PACKAGE_NAME@.tests;
> +
> +import @ANDROID_PACKAGE_NAME@.*;
> +
> +class StringHelper {

I love this class.

Comment 6

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch

Made sure the changes to addOrUpdateMobileBookmark are ok. 
Added the correct implementation with reflections for updateBookmark. 
Renamed the getFolderId method to protected long getFolderIdFromGuid(String guid) - we can log a follow-up bug to modify the method in BrowserDB to public
Changed all the Log.e/Log.d to assertions and the constructor to receive the mAsserter from BaseTest so we don't need to set it up separately here
Made changes in a few tests that were accessing moved methods/ strings/ resources

I did not move:
    protected enum AboutHomeTabs {MOST_VISITED, MOST_RECENT, TABS_FROM_LAST_TIME, BOOKMARKS, READING_LIST};
    protected ArrayList<String> aboutHomeTabs = new ArrayList<String>() {{
                  add("HISTORY");
                  add("BOOKMARKS");
                  add("READING_LIST");
              }};
 since they are used only in AboutHomeTest and this would just complicate the openAboutHomeTab method.

Comment 7

[Adrian Tamas (:AdrianT)]

Attachment: proof of concept test to make sure each method in DataBaseHelper works

Comment 8

[:Margaret Leibovic]

Comment on attachment 797178 [details] [diff] [review]
robocopRestructure.patch

Sorry I've been so slow about reviewing this :(

I think we should just move over the current updateBookmark method from AboutHomeTest, and we don't need to include this getBookmarkId method, since it's currently only used in your implementation of updateBookmark.

I really promise to be faster about reviewing a new version of this patch.

Comment 9

[:Margaret Leibovic]

Cc'ing liuche, since she's been doing a lot of robocop work recently and would be interested in this bug.

Comment 10

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch v2

Switched the updateBookmark method to the one currently in AboutHomeTest and removed the getBookmarkId method. Also made changes so the patch can be applied on the latest sources without conflicts

Comment 11

[:Margaret Leibovic]

Comment on attachment 802362 [details] [diff] [review]
robocopRestructure.patch v2

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

Thanks for addressing my comments. I have a few nits, but we can push this once we have a green try run. I also have a few ideas for follow-up work, but I don't want to hold up landing this patch, since it's definitely an improvement.

Also, when we land this, I think it would be good to make a post to mobile-firefox-dev, particularly about the StringHelper changes, so that people know to use that in the future when writing tests.

::: mobile/android/base/tests/BaseTest.java.in
@@ +67,5 @@
>      private String mLogFile;
>      protected String mProfile;
>      public Device mDevice;
> +    protected DatabaseHelper mDatabaseHelper;
> +    protected StringHelper mStringHelper;

In the future it could be nice to try to make these classes with static methods, so that we don't need to hold references to them like this, similar to some of the things lucasr was doing in bug 910859.

We don't need to do that now, but maybe we should file a follow-up bug for that.

::: mobile/android/base/tests/DatabaseHelper.java.in
@@ +13,5 @@
> +
> +class DatabaseHelper {
> +    public enum BrowserDataType {BOOKMARKS, HISTORY};
> +    Activity mActivity;
> +    Assert mAsserter;

It looks like these can be private, so let's make them private.

@@ +125,5 @@
> +            mAsserter.ok(false, "Exception deleting history item", e.toString());
> +        }
> +    }
> +
> +    // About the same implementation as getFolderIdFromGuid from LocalBrowserDB because it is declared private and we can't use reflections to access it

I'd also like to file a bug about cleaning up LocalBrowserDB to make it easier for test methods to access things they need. I talked to wesj about this, and I think we decided that it would be worthwhile to make methods public and/or edit their signatures if it makes it easier for tests to just use that logic.

::: mobile/android/base/tests/StringHelper.java.in
@@ +82,5 @@
> +    public static final String PRODUCT_ANNOUNCEMENTS_LABEL = "Show product announcements";
> +    public static final String LOCATION_SERVICES_LABEL = "Mozilla location services";
> +    public static final String HELTH_REPORT_LABEL = "(Fennec|Nightly|Aurora|Firefox Beta|Firefox) Health Report";
> +    public static final String MY_HEALTH_REPORT_LABEL = "View my Health Report";
> +    

Nit: whitespace.

Comment 12

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch v2.1

(In reply to :Margaret Leibovic from comment #11)
> Comment on attachment 802362 [details] [diff] [review]
> robocopRestructure.patch v2
> 
> Review of attachment 802362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for addressing my comments. I have a few nits, but we can push this
> once we have a green try run. I also have a few ideas for follow-up work,
> but I don't want to hold up landing this patch, since it's definitely an
> improvement.
> 
> Also, when we land this, I think it would be good to make a post to
> mobile-firefox-dev, particularly about the StringHelper changes, so that
> people know to use that in the future when writing tests.

I am going to send out an email on the mobile-firefox-dev and qa-mobile mailing lists

> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +67,5 @@
> >      private String mLogFile;
> >      protected String mProfile;
> >      public Device mDevice;
> > +    protected DatabaseHelper mDatabaseHelper;
> > +    protected StringHelper mStringHelper;
> 
> In the future it could be nice to try to make these classes with static
> methods, so that we don't need to hold references to them like this, similar
> to some of the things lucasr was doing in bug 910859.
> 
> We don't need to do that now, but maybe we should file a follow-up bug for
> that.

Follow-up bug logged: bug 915005

> ::: mobile/android/base/tests/DatabaseHelper.java.in
> @@ +13,5 @@
> > +
> > +class DatabaseHelper {
> > +    public enum BrowserDataType {BOOKMARKS, HISTORY};
> > +    Activity mActivity;
> > +    Assert mAsserter;
> 
> It looks like these can be private, so let's make them private.

BrowserDataType is used to determine what info to get from the database via the getBrowserDBUrls method. I made it protected just to restrict access as much as possible but I can't make it private

> @@ +125,5 @@
> > +            mAsserter.ok(false, "Exception deleting history item", e.toString());
> > +        }
> > +    }
> > +
> > +    // About the same implementation as getFolderIdFromGuid from LocalBrowserDB because it is declared private and we can't use reflections to access it
> 
> I'd also like to file a bug about cleaning up LocalBrowserDB to make it
> easier for test methods to access things they need. I talked to wesj about
> this, and I think we decided that it would be worthwhile to make methods
> public and/or edit their signatures if it makes it easier for tests to just
> use that logic.

Follow-up bug logged: bug 915022

Try run for the patch: https://tbpl.mozilla.org/?tree=Try&rev=d0a8a2a2e36e

Comment 13

[:Margaret Leibovic]

(In reply to Adrian Tamas (:AdrianT) from comment #12)

> > ::: mobile/android/base/tests/DatabaseHelper.java.in
> > @@ +13,5 @@
> > > +
> > > +class DatabaseHelper {
> > > +    public enum BrowserDataType {BOOKMARKS, HISTORY};
> > > +    Activity mActivity;
> > > +    Assert mAsserter;
> > 
> > It looks like these can be private, so let's make them private.
> 
> BrowserDataType is used to determine what info to get from the database via
> the getBrowserDBUrls method. I made it protected just to restrict access as
> much as possible but I can't make it private

I was referring to mActivity and mAsserter. Is there a reason those can't be private?

Comment 14

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch v2.2

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Adrian Tamas (:AdrianT) from comment #12)
> 
> > > ::: mobile/android/base/tests/DatabaseHelper.java.in
> > > @@ +13,5 @@
> > > > +
> > > > +class DatabaseHelper {
> > > > +    public enum BrowserDataType {BOOKMARKS, HISTORY};
> > > > +    Activity mActivity;
> > > > +    Assert mAsserter;
> > > 
> > > It looks like these can be private, so let's make them private.
> > 
> > BrowserDataType is used to determine what info to get from the database via
> > the getBrowserDBUrls method. I made it protected just to restrict access as
> > much as possible but I can't make it private
> 
> I was referring to mActivity and mAsserter. Is there a reason those can't be
> private?

Yeah sorry I misunderstood which variables I should set private. Changed mActivity and mAsserter to private. The original try run looks green. Can you please check-in the patch.

Comment 15

[:Margaret Leibovic]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/c24ba14732cd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android bustage.
https://hg.mozilla.org/integration/fx-team/rev/e86f8332bab1

https://tbpl.mozilla.org/php/getParsedLog.php?id=27775190&tree=Fx-Team

Comment 17

[Adrian Tamas (:AdrianT)]

Attachment: robocopRestructure.patch v3

I didn't account for the patch for bug 896557 being integrated which used methods from AboutHomeTest that were moved. Remade the patch from the latest sources.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9d3fc8abb2ca

Comment 18

[Adrian Tamas (:AdrianT)]

Comment on attachment 804405 [details] [diff] [review]
robocopRestructure.patch v3

Minor changes to account for the integration of the testLinkContextMenu, testPictureLinkContextMenu and testMailToContextMenu tests which caused the build error. Please repush the patch if the review is ok.

Comment 19

[:Margaret Leibovic]

Comment on attachment 804405 [details] [diff] [review]
robocopRestructure.patch v3

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

::: mobile/android/base/tests/testLinkContextMenu.java.in
@@ -31,5 @@
>          verifyShareOption(linkMenuItems[3], LINK_PAGE_TITLE); // Test the "Share Link" option
>          verifyBookmarkLinkOption(linkMenuItems[4], BLANK_PAGE_URL); // Test the "Bookmark Link" option
>      }
>  
> -    @Override

We should keep this @Override annotation. I can land the patch with this addressed.

Comment 20

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/c33d415d991e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c33d415d991e