833832 - Robocop: Add test for Import from Android feature

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test

This bug will cover the progress on a Robocop test to test the Import Bookmarks and History from Android feature.

Moztrap tests:
https://moztrap.mozilla.org/manage/case/789/ Import Bookmarks & History from Android
https://moztrap.mozilla.org/manage/case/805/ Importing stock Bookmarks & History twice shouldn't generate duplicate entries

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 705384 [details] [diff] [review]
Import bookmarks and history from Android test

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

partially reviewed, r- due to lack of cleanup.  I will run some tests tonight tomorrow and update with results and any other feedback.

::: mobile/android/base/tests/testImportFromAndroid.java.in
@@ +48,5 @@
> +
> +        // Verify bookmarks are imported
> +        firefoxBookmarks = getFirefoxBookmarks();
> +        for (String url:androidBookmarks) {
> +             mAsserter.ok(firefoxBookmarks.contains(url), "Checking if Android Bookmark is pressent", " Android Bookmark " + url + " was imported");

nit: s/pressent/present/

@@ +57,5 @@
> +        firefoxHistory = getFirefoxHistory();
> +        for (String url:androidHistory) {
> +             if (!androidBookmarks.contains(url)) { // For the stock browser Bookmarks appear in the history list
> +                 mAsserter.ok(firefoxHistory.contains(url), "Checking if Android History is pressent", " Android History item " + url + " was imported");
> +             }

maybe we could just query history and compare either to firefoxBookmarks or firefoxHistory.  It would make the code a bit simpler.

@@ +62,5 @@
> +        }
> +
> +        // Verify the original Firefox Bookmarks are not deleted
> +        for (String url:oldFirefoxBookmarks) {
> +             mAsserter.ok(oldFirefoxBookmarks.contains(url), "Checking if original Firefox Bookmark is pressent", " Firefox Bookmark " + url + " was not removed");

you are checking the old against the old, I assume you want to do:
mAsserter.ok(firefoxBookmarks.contains(url)...

@@ +68,5 @@
> +
> +        // Verify the original Firefox History is not deleted
> +        for (String url:oldFirefoxHistory) {
> +             mAsserter.ok(oldFirefoxHistory.contains(url), "Checking original Firefox History item is pressent", " Firefox History item " + url + " was not removed");
> +        }

you are checking the old against the old, I assume you want to do:
mAsserter.ok(firefoxHistory.contains(url)...

@@ +87,5 @@
> +        }
> +
> +        // Verify history count is not increased after the second import
> +        mAsserter.ok(firefoxHistory.size() == getFirefoxHistory().size(), "The number of history entries was not increased", "None of the items where duplicated");
> +

can we cleanup at the end?

@@ +115,5 @@
> +        try {
> +            if (data.equals("Bookmarks")) {
> +                cursor = mBrowser.getAllBookmarks(resolver);
> +            } else 
> +            if (data.equals("History")) {

can you put 'else if' on the same line?

Comment 2

[Adrian Tamas (:AdrianT)]

(In reply to Joel Maher (:jmaher) from comment #1)
> Comment on attachment 705384 [details] [diff] [review]
> Import bookmarks and history from Android test
> 
> Review of attachment 705384 [details] [diff] [review]:
> @@ +87,5 @@
> > +        }
> > +
> > +        // Verify history count is not increased after the second import
> > +        mAsserter.ok(firefoxHistory.size() == getFirefoxHistory().size(), "The number of history entries was not increased", "None of the items where duplicated");
> > +
> 
> can we cleanup at the end?
I am deleting the bookmarks in the TearDown method using the deleteImportedData() method. Isn't it ok like this? I could just clear private data from the menu.

I will work on the other changes and upload a new patch asap

Comment 3

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v2

Made the necessary changes to clean up the test. Merged getFirefoxBookmarks and getFirefoxHistory to one method that returns information based on if the user asks for bookmarks or history, added the verification of the import only from the android history since it contains both the bookmarks and the history. I kept the clearing of imported data in the tearDown method and by adding extra logging I see that the imported bookmarks and history are removed

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 705859 [details] [diff] [review]
Import bookmarks and history from Android test v2

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

I still need to test this on try, this passes locally for me and I just have minor tweaks below.

::: mobile/android/base/tests/testImportFromAndroid.java.in
@@ +41,5 @@
> +        oldFirefoxBookmarks = getFirefoxData("bookmarks");
> +        oldFirefoxHistory = getFirefoxData("history");
> +
> +        // Import the bookmarks and history
> +        importDataFromAndroid();

what data is expected in the android data?  Can we preseed it with data?  some known data which might overlap a little with firefox also?

@@ +90,5 @@
> +        Device mDevice = new Device();
> +        selectMenuItem("Settings");
> +        mSolo.waitForText("General");
> +        mSolo.clickOnText("Import from Android");
> +        mSolo.waitForText("Import from Android");

why do you click on the text, then wait for it, please add a comment.

@@ +93,5 @@
> +        mSolo.clickOnText("Import from Android");
> +        mSolo.waitForText("Import from Android");
> +
> +        // The Import button is the first on Gingerbread and the second in Honeycomb and newer
> +        if (mDevice.version.equals("2.x")) { 

nit, white space after the {

@@ +98,5 @@
> +            mSolo.clickOnButton(0);
> +        } else {
> +            mSolo.clickOnButton(1);
> +        }
> +        mSolo.waitForText("Import&Export"); // Import has finished

are there any concerns with the & character?

@@ +117,5 @@
> +            }
> +            if (cursor != null) {
> +                cursor.moveToFirst();
> +                for (int i = 0; i < cursor.getCount(); i++ ) {
> +                     urls.add(cursor.getString(cursor.getColumnIndex("url")));

any concern here with url == null as we do below in getFirefoxData()

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

good news this passes on try server.

Comment 6

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v2.1

(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 705859 [details] [diff] [review]
> Import bookmarks and history from Android test v2
> 
> Review of attachment 705859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still need to test this on try, this passes locally for me and I just have
> minor tweaks below.
> 
> ::: mobile/android/base/tests/testImportFromAndroid.java.in
> @@ +41,5 @@
> > +        oldFirefoxBookmarks = getFirefoxData("bookmarks");
> > +        oldFirefoxHistory = getFirefoxData("history");
> > +
> > +        // Import the bookmarks and history
> > +        importDataFromAndroid();
> 
> what data is expected in the android data?  Can we preseed it with data? 
> some known data which might overlap a little with firefox also?
I created the addData method to grab every 3rd bookmark and history url and add it to the Firefox database to create an overlap of data. Unfortunately I am unable to add data to the Android Stock Browser database. I have tried:
1) Adding bookmarks via Browser.saveBookmark(Context,title, url) method but this opens a pop-up generated by the Android Browser in which I can't tap the buttons - a different app and robotium can't test over multiple apps
2) I have tried entering data in the database using a ContentResolver but the app does not have permissions to write bookmarks and history. I can't add these to the main app (After a discussion with Mark Finkle permissions can;t be added just because of automation tests) and setting the permission just for the Robocop apk does not work
3) I have tried adding data via sqlite but on a rooted device using adb commands from a terminal I still don't have permissions to access sqlite in the /data/data directory - I have not investigated this more since I am unable to access the browser data from adb
Maybe file a follow-up to find a way to insert data here? Maybe someone with more knowledge of the test harness code has any idea on what can be done here?
> 
> @@ +90,5 @@
> > +        Device mDevice = new Device();
> > +        selectMenuItem("Settings");
> > +        mSolo.waitForText("General");
> > +        mSolo.clickOnText("Import from Android");
> > +        mSolo.waitForText("Import from Android");
> 
> why do you click on the text, then wait for it, please add a comment.

The pop-up and the option have the same name. I added a comment in the code

> @@ +98,5 @@
> > +            mSolo.clickOnButton(0);
> > +        } else {
> > +            mSolo.clickOnButton(1);
> > +        }
> > +        mSolo.waitForText("Import&Export"); // Import has finished
> 
> are there any concerns with the & character?

I have not seen any issues with "&". I could just change it to "Export" if needed.

> @@ +117,5 @@
> > +            }
> > +            if (cursor != null) {
> > +                cursor.moveToFirst();
> > +                for (int i = 0; i < cursor.getCount(); i++ ) {
> > +                     urls.add(cursor.getString(cursor.getColumnIndex("url")));
> 
> any concern here with url == null as we do below in getFirefoxData()

I am checking for url==null in the getFirefoxData method because the folders in the Firefox Bookmarks hierarchy do not have a url. This problem does not occur on the stock browser

Comment 7

[Geoff Brown [:gbrown]]

Comment on attachment 707629 [details] [diff] [review]
Import bookmarks and history from Android test v2.1

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

Stepping in for the review since :jmaher is away for a few days.

>> what data is expected in the android data?  Can we preseed it with data? 
>> some known data which might overlap a little with firefox also?
> I created the addData method to grab every 3rd bookmark and history url 
> and add it to the Firefox database to create an overlap of data. 
> Unfortunately I am unable to add data to the Android Stock 
> Browser database. I have tried:
> 1) Adding bookmarks via Browser.saveBookmark(Context,title, url) method 
> but this opens a pop-up generated by the Android Browser in which I can't 
> tap the buttons - a different app and robotium can't test over multiple apps

Right. That's probably not possible.

> 2) I have tried entering data in the database using a ContentResolver but 
> the app does not have permissions to write bookmarks and history. I can't 
> add these to the main app (After a discussion with Mark Finkle permissions 
> can;t be added just because of automation tests) and setting the permission 
> just for the Robocop apk does not work

Agreed -- we cannot be adding permissions just to support tests.

> 3) I have tried adding data via sqlite but on a rooted device using adb 
> commands from a terminal I still don't have permissions to access sqlite 
> in the /data/data directory - I have not investigated this more since I 
> am unable to access the browser data from adb

That's interesting. I wonder why root would still not have permission.
Can you investigate a bit more?

> Maybe file a follow-up to find a way to insert data here? Maybe someone 
> with more knowledge of the test harness code has any idea on what can be done here?

I think this is an important feature of this test; I would prefer to see this issue
resolved before we start running this test.

::: mobile/android/base/tests/testImportFromAndroid.java.in
@@ +42,5 @@
> +
> +        // Get the initial bookmarks and history
> +        oldFirefoxBookmarks = getFirefoxData("bookmarks");
> +        oldFirefoxHistory = getFirefoxData("history");
> +        

nit - remove the extra space here

@@ +84,5 @@
> +             }
> +        }
> +
> +        // Verify history count is not increased after the second import
> +        mAsserter.ok(firefoxHistory.size() == getFirefoxData("history").size(), "The number of history entries was not increased", "None of the items where duplicated");

nit: "where" -> "were"

@@ +116,5 @@
> +            }
> +        }*/
> +        ContentResolver resolver = getActivity().getContentResolver();
> +        for (int i = 0; i < 10; i++) {
> +            addOrUpdateBookmark("Bookmar Number" + String.valueOf(i), "http://www.bookmark" + String.valueOf(i) +".com", true);

nit - "Bookmar" -> "Bookmark" ... unless this is intentional, for some reason?

@@ +122,5 @@
> +            values.put("title", "Bookmar Number" + String.valueOf(i));
> +            values.put("url", "http://www.bookmark" + String.valueOf(i) +".com");
> +            values.put("folder", 0);
> +            values.put("parent", 0);
> +            resolver.insert(Browser.BOOKMARKS_URI, values);

As you noted, this throws a SecurityException, due to missing WRITE_HISTORY_BOOKMARKS permission.

@@ +135,5 @@
> +        mSolo.waitForText("General");
> +        mSolo.clickOnText("Import from Android");
> +
> +        // Wait for the Import form Android popup to be opened. It has the same title as the option
> +        mSolo.waitForText("Import from Android");

Thanks for adding the note. 

There's a danger here that the waitForText will succeed immediately, on the "Import from Android" text from the Settings. Would it be possible to wait for the Import from Android popup based on a view id, or something like that, instead?

@@ +139,5 @@
> +        mSolo.waitForText("Import from Android");
> +
> +        // The Import button is the first on Gingerbread and the second in Honeycomb and newer
> +        if (mDevice.version.equals("2.x")) {
> +            mSolo.clickOnButton(0);

I think you can use mSolo.clickOnButton("Import") here -- that should work on all devices.

@@ +143,5 @@
> +            mSolo.clickOnButton(0);
> +        } else {
> +            mSolo.clickOnButton(1);
> +        }
> +        mSolo.waitForText("Import&Export"); // Import has finished

If I follow these steps manually, I never see "Import&Export" displayed...I'm not sure this is an effective way to wait for completion (but I don't know what to suggest instead).

@@ +196,5 @@
> +                    if (cursor.getString(cursor.getColumnIndex("url")) != null) {
> +                        firefoxData.add(cursor.getString(cursor.getColumnIndex("url")));
> +                    }
> +                    if(!cursor.isLast()) {
> +                            cursor.moveToNext();

nit - indent level is off here

Comment 8

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v3

Comment 9

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v3.1

Cleaned up some extra spaces left in the previous patch.

In the previous version I had forgotten to refresh the patch so the unusable code to add data in the android bookmarks was still present. Cleaned up the addData() method so know it just takes each 3rd bookmark and each 3rd history item and also adds it in the Firefox Mobile Database so we have some data overlap between Firefox and the Android Browser before the import. I also cleaned up the other minor issues spotted by Geoff.

As discussed with Joel on IRC I looked into adding the data to the Android Browser Database from the test harness and in particular the SUTAgentAndroid was suggested by him.

I have rooted a device and tried to add the data using sqlite. Unfortunately the command failed with a syntax error: '"(" unexpected', failing at the point where I was specifying the bookmarks table columns, so I have changed the approach. 

Since the SUTAgent is only used in the test environment I have added the permissions to read and write history and bookmarks to the sutAgentAndroid apk, which should not affect the release builds, and on creation of the SUTAgent I call the new insertBookmarksInAndroidDB method from DoCommand. The method inserts the bookmarks using the ContentResolver since now the apk has the correct permissions. 

Before adding 20 bookmarks the method checks to make sure that the url has not been previously bookmarked just to make sure that this bookmarks are not created and duplicated each time the SUTAgent is started.

Comment 10

[Geoff Brown [:gbrown]]

What is the plan for devicemanagerADB users? If a developer runs this test locally via ADB, the test will behave differently...that might be confusing.

Comment 11

[Adrian Tamas (:AdrianT)]

(In reply to Geoff Brown [:gbrown] from comment #10)
> What is the plan for devicemanagerADB users? If a developer runs this test
> locally via ADB, the test will behave differently...that might be confusing.

This was a just a solution to add test data to the panda/tegra boards. This would be different for local tests regardless if the user imports the bookmarks or not using the SUTAgent. This test is dynamic depending on the users data. One user may have 100 history items in the Android Browser and another may have 10 this is why I get the hole list of bookmarks and history from Android and check each item separately.

If you consider that this solution is still not the appropriate one I can investigate this further.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 710647 [details] [diff] [review]
Import bookmarks and history from Android test v3.1

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

A bunch of nits for the test case, but r- for hard coding this in the sutagent.

I was very impressed to see the creative use of our toolchain.  I did test this locally on my tegra and panda board, all tests pass consistently!   So you are are the right path.  What I don't like about the sutagent approach is:
1) this inserts bookmarks on start (and it will not if it finds duplicates), it should be a command we call 
2) the requires us to update the sutagent on all the devices
3) as Geoff pointed out, there is no support for adb, this is what developers use while debugging tests locally on personal development devices (phones or boards)

I would prefer to work with you and figure out a solution for inserting via sqlite on the commandline on a rooted phone.  We might be able to do other tricks as well, I am just not sure all of our options.

Thanks again for the creativity and passing tests.  I am glad we are down to one issue left before calling this good.

::: mobile/android/base/tests/testImportFromAndroid.java.in
@@ +55,5 @@
> +         * Verify the history and bookmarks are imported
> +         * Android history also contains the android bookmarks so we don't need to get them separately here
> +         */
> +        for (String url:androidData) {
> +            mAsserter.ok(firefoxHistory.contains(url)||firefoxBookmarks.contains(url), "Checking if Android" + (firefoxBookmarks.contains(url) ? " Bookmark" : " History item") + " is present", url + " was imported");

nit: s/" is present"/" is present "/

@@ +94,5 @@
> +        // Add a few Bookmarks from Android to Firefox Mobile
> +        for (String url:androidBookmarks) {
> +            // Add every 3rd bookmark to Firefox Mobile
> +            if ((androidBookmarks.indexOf(url) % 3) == 0) {
> +                 addOrUpdateBookmark("Bookmar Number" + String.valueOf(androidBookmarks.indexOf(url)), url, true);

s/Bookmar/Bookmark/

@@ +146,5 @@
> +                cursor = mBrowser.getAllBookmarks(resolver);
> +            }
> +            if (cursor != null) {
> +                cursor.moveToFirst();
> +                for (int i = 0; i < cursor.getCount(); i++ ) {

nit: extra space between i++ and )

@@ +149,5 @@
> +                cursor.moveToFirst();
> +                for (int i = 0; i < cursor.getCount(); i++ ) {
> +                     urls.add(cursor.getString(cursor.getColumnIndex("url")));
> +                     if(!cursor.isLast()) {
> +                        cursor.moveToNext();

this looks like a 3 space instead of 4 space indentation.

@@ +172,5 @@
> +        try {
> +            cursor = resolver.query(uri, null, null, null, null);
> +            if (cursor != null) {
> +                cursor.moveToFirst();
> +                for (int i = 0; i < cursor.getCount(); i++ ) {

nit: extra space between i++ and )

@@ +181,5 @@
> +                    if (cursor.getString(cursor.getColumnIndex("url")) != null) {
> +                        firefoxData.add(cursor.getString(cursor.getColumnIndex("url")));
> +                    }
> +                    if(!cursor.isLast()) {
> +                            cursor.moveToNext();

nit: 4 space indent please.

Comment 13

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v4.0

Link to try: https://tbpl.mozilla.org/?tree=Try&rev=7bc240cf0fb7 although at this time it did not start the build process for some reason

Made the neccessary nit changes.

As suggested I made the changes in the runtestremote.py script to check if the test is testImportFromAndroid and insert the data in the android db. I start adding data starting from id 30 and from position 100 - this should ensure that at least on the panda/tegra boards we will not overwright any bookmarks. Also by specifing the id we can do an "insert or replace" to make sure that we don't always insert bookmarks regardless of if they exist or not.

This only works when using runtestsremote with the devicemanagerSUT. The _runCmds is not present in the devicemagerADB and as we taked on IRC we could not get this to work with the "shell" command. I also tryed to do it with _sendCmd from devicemangerADB but the commands behave completely different.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 725408 [details] [diff] [review]
Import bookmarks and history from Android test v4.0

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

I still need to test this, but overall I am concerned about inserting into the system android database and not cleaning it up.

::: mobile/android/base/tests/testImportFromAndroid.java.in
@@ +104,5 @@
> +        Uri uri = Uri.parse("content://@ANDROID_PACKAGE_NAME@.db.browser/history");
> +        uri = uri.buildUpon().appendQueryParameter("profile", "default")
> +                             .appendQueryParameter("sync", "true").build();
> +        for (String url:androidData) {
> +            // Add every 3rd website from Android History to Firefox Mobile

why every 3rd website?

::: testing/mochitest/runtestsremote.py
@@ +564,5 @@
>  
> +            # If the test is for checking the import from bookmarks then make sure there is data to import
> +            if test['name'] == "testImportFromAndroid":
> +                for i in range(20):
> +                   cmd = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'insert or replace into bookmarks(_id,title,url,folder,parent,position) values (" + str(30 + i) + ",\"Bookmark"+ str(i) + "\",\"www.bookmark" + str(i) + ".com\",0,1," + str(100 + i) + ");'"]

do we ever remove these bookmarks?  Is there a chance that we will slow the system down by having thousands of bookmarks over time?

@@ +566,5 @@
> +            if test['name'] == "testImportFromAndroid":
> +                for i in range(20):
> +                   cmd = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'insert or replace into bookmarks(_id,title,url,folder,parent,position) values (" + str(30 + i) + ",\"Bookmark"+ str(i) + "\",\"www.bookmark" + str(i) + ".com\",0,1," + str(100 + i) + ");'"]
> +                   if (options.dm_trans == "sut"):
> +                       dm._runCmds([{"cmd": " ".join(cmd)}])                      

nit: extra whitespace after the above line

Comment 15

[Adrian Tamas (:AdrianT)]

(In reply to Joel Maher (:jmaher) from comment #14)
> Comment on attachment 725408 [details] [diff] [review]
> Import bookmarks and history from Android test v4.0
> 
> Review of attachment 725408 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still need to test this, but overall I am concerned about inserting into
> the system android database and not cleaning it up.
I can add some printing in the logs? not sure if this can be made interactive since it's supposed to be run automated....
> 
> ::: mobile/android/base/tests/testImportFromAndroid.java.in
> @@ +104,5 @@
> > +        Uri uri = Uri.parse("content://@ANDROID_PACKAGE_NAME@.db.browser/history");
> > +        uri = uri.buildUpon().appendQueryParameter("profile", "default")
> > +                             .appendQueryParameter("sync", "true").build();
> > +        for (String url:androidData) {
> > +            // Add every 3rd website from Android History to Firefox Mobile
> 
> why every 3rd website?
My thought here is that if there are around 30 bookmarks in the android db (~10 default and 20 inserted by me) this would add 10 bookmarks that would be present in both the android browser and firefox at the moment of the import and a few others - 4 on firefox and ~ 20 on android. This seems a decent split of possible options

> ::: testing/mochitest/runtestsremote.py
> @@ +564,5 @@
> >  
> > +            # If the test is for checking the import from bookmarks then make sure there is data to import
> > +            if test['name'] == "testImportFromAndroid":
> > +                for i in range(20):
> > +                   cmd = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'insert or replace into bookmarks(_id,title,url,folder,parent,position) values (" + str(30 + i) + ",\"Bookmark"+ str(i) + "\",\"www.bookmark" + str(i) + ".com\",0,1," + str(100 + i) + ");'"]
> 
> do we ever remove these bookmarks?  Is there a chance that we will slow the
> system down by having thousands of bookmarks over time?
Using "insert or replace" ensures that there will be only 20 bookmarks added which sould not affect the performance at all. We do not create extra if it is not needed
> 
> @@ +566,5 @@
> > +            if test['name'] == "testImportFromAndroid":
> > +                for i in range(20):
> > +                   cmd = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'insert or replace into bookmarks(_id,title,url,folder,parent,position) values (" + str(30 + i) + ",\"Bookmark"+ str(i) + "\",\"www.bookmark" + str(i) + ".com\",0,1," + str(100 + i) + ");'"]
> > +                   if (options.dm_trans == "sut"):
> > +                       dm._runCmds([{"cmd": " ".join(cmd)}])                      
> 
> nit: extra whitespace after the above line

Comment 16

[Adrian Tamas (:AdrianT)]

(In reply to adrian tamas from comment #15)
> (In reply to Joel Maher (:jmaher) from comment #14)
> > Comment on attachment 725408 [details] [diff] [review]
> > Import bookmarks and history from Android test v4.0
> > 
> > Review of attachment 725408 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I still need to test this, but overall I am concerned about inserting into
> > the system android database and not cleaning it up.
> I can add some printing in the logs? not sure if this can be made
> interactive since it's supposed to be run automated....
Please disregard this comment. I understood the original comment wrong on the first read.

Comment 17

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v4.1

Added the database cleanup at the end of the test to remove the added bookmarks at the begining of the test. Although the bookmarks would have not increased in number on subsequent runs I make sure to remove each bookmark I added.
 
The patch passes localy without any issues but seems to fail on Tegras on the tryserver today in the selectMenuItem method when opening "Settings" now whitout any changes to the java test. I know this fail is intermitent. Please retest on try if needed.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 726178 [details] [diff] [review]
Import bookmarks and history from Android test v4.1

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

r-

failures on try server and we need to ensure we delete these bookmarks as best as possible.

::: testing/mochitest/runtestsremote.py
@@ +581,5 @@
> +                    if test['name'] == "testImportFromAndroid":
> +                        cmd_del = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'delete from bookmarks where title like \"%Bookmark%\"'"]
> +                        print cmd_del
> +                        if (options.dm_trans == "sut"):
> +                            dm._runCmds([{"cmd": " ".join(cmd_del)}])

I would like to see this after the except clause (maybe a finally clause?)

Comment 19

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v4.2

Added the bookmark clearing in a finaly statment in runtestsremote.py
Added extra waitForText in importFromAndroid to make sure the menu is opened. I believe the fails on try of the last patch were caused by the fact that the menu was not opened when sending the MENU key althout the test was correctly possitioned to open the menu

Run on try: https://tbpl.mozilla.org/?tree=Try&rev=0371c9c4f7b7

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 726719 [details] [diff] [review]
Import bookmarks and history from Android test v4.2

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

thanks.  I see a lot of success with this on try server as well.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Looking on try server this is not that stable, it works great on the panda board, but 50% on the tegra:
https://tbpl.mozilla.org/?tree=Try&rev=0371c9c4f7b7

Comment 22

[Adrian Tamas (:AdrianT)]

It seems that on the tegras where the test fails there where mochi and talos tests run on the Stock Android Browser which left history entries which may be problematic to import. I have talked to :gcp on IRC and although this fails it shouldn't as entries are copied as they are from the Android Database. I manually checked this with a local file on the HTC Desire (Android 2.2) and it does not seem to be an issue as the entry is imported.
I could investigate this further or as an alternative I could check if the entry was imported only if it starts with "http" which would indicate a website. This would not be the ideal solution but would solve the fails.
Another issue is that the Stock Browser has a different database structure on Android 2.3(Tegra) and 4.0.4(Panda) so the bookmark inserts fail on tegra. The good news is that there are a few default bookmarks on tegra and we do have data to test with. From the logs for the default Android Bookmarks:
03-19 14:19:28.489 I/Robocop ( 6965): 7 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.google.com/ was imported
03-19 14:19:28.489 I/Robocop ( 6965): 8 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://picasaweb.google.com/m/viewer?source=androidclient was imported
03-19 14:19:28.489 I/Robocop ( 6965): 9 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.yahoo.com/ was imported
03-19 14:19:28.489 I/Robocop ( 6965): 10 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.msn.com/ was imported
03-19 14:19:28.489 I/Robocop ( 6965): 11 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.myspace.com/ was imported
03-19 14:19:28.489 I/Robocop ( 6965): 12 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.facebook.com/ was imported
03-19 14:19:28.489 I/Robocop ( 6965): 13 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.wikipedia.org/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 14 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.ebay.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 15 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.cnn.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 16 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.nytimes.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 17 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://espn.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 18 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.amazon.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 19 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.weather.com/ was imported
03-19 14:19:28.499 I/Robocop ( 6965): 20 INFO TEST-PASS | testImportFromAndroid | Checking if Android Bookmark is present - http://www.bbc.co.uk/ was imported

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for the explanation here.  If we don't get the preseed bookmarks in the tegras, then I assume there could be errors in the future as this test morphs or other changes in the browser affect this.

I am not a fan of writing special case code for different versions, but we might need to do that.  The most unfriendly part of this is that this special case code would need to live in the python harness :(

Solving this right now instead of taking shortcuts is the right thing to do.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

Using the latest patch from the most recent try server run it fails on the tegra, but looking at it locally it is due to the fact that there are existing non default bookmarks in the database.

I would recommend flushing the android database before we start the test, and cleaning up afterwards.

The best method would be:
tegra:
delete from bookmarks where _id>14;

panda:
delete from bookmarks where _id>15;

Would we want to just make it the same and delete one of the defaults from the panda board. Here is what we have on the panda board:

sqlite> .dump bookmarks
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE bookmarks(_id INTEGER PRIMARY KEY AUTOINCREMENT,title TEXT,url TEXT,folder INTEGER NOT NULL DEFAULT 0,parent INTEGER,position INTEGER NOT NULL,insert_after INTEGER,deleted INTEGER NOT NULL DEFAULT 0,account_name TEXT,account_type TEXT,sourceid TEXT,version INTEGER NOT NULL DEFAULT 1,created INTEGER,modified INTEGER,dirty INTEGER NOT NULL DEFAULT 0,sync1 TEXT,sync2 TEXT,sync3 TEXT,sync4 TEXT,sync5 TEXT);
INSERT INTO "bookmarks" VALUES(1,'Bookmarks',NULL,1,NULL,0,NULL,0,NULL,NULL,NULL,1,NULL,NULL,1,NULL,NULL,'google_chrome_bookmarks',NULL,NULL);
INSERT INTO "bookmarks" VALUES(2,'Google','http://www.google.com/',0,1,0,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(3,'Picasa','http://picasaweb.google.com/',0,1,2,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(4,'Yahoo!','http://www.yahoo.com/',0,1,4,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(5,'MSN','http://www.msn.com/',0,1,6,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(6,'Twitter','http://twitter.com/',0,1,8,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(7,'Facebook','http://www.facebook.com/',0,1,10,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(8,'Wikipedia','http://www.wikipedia.org/',0,1,12,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(9,'eBay','http://www.ebay.com/',0,1,14,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(10,'CNN','http://www.cnn.com/',0,1,16,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(11,'NY Times','http://www.nytimes.com/',0,1,18,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(12,'ESPN','http://espn.com/',0,1,20,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(13,'Amazon','http://www.amazon.com/',0,1,22,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(14,'Weather Channel','http://www.weather.com/',0,1,24,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(15,'BBC','http://www.bbc.co.uk/',0,1,26,NULL,0,NULL,NULL,NULL,1,1340810616798,NULL,0,NULL,NULL,NULL,NULL,NULL);
COMMIT;
sqlite>


Here is what we have on the tegra:
.dump bookmarks
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE bookmarks (_id INTEGER PRIMARY KEY,title TEXT,url TEXT,visits INTEGER,date LONG,created LONG,description TEXT,bookmark INTEGER,favicon BLOB DEFAULT NULL,thumbnail BLOB DEFAULT NULL,touch_icon BLOB DEFAULT NULL,user_entered INTEGER);
INSERT INTO "bookmarks" VALUES(1,'Google','http://www.google.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(2,'Picasa','http://picasaweb.google.com/m/viewer?source=androidclient',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(3,'Yahoo!','http://www.yahoo.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(4,'MSN','http://www.msn.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(5,'MySpace','http://www.myspace.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(6,'Facebook','http://www.facebook.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(7,'Wikipedia','http://www.wikipedia.org/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(8,'eBay','http://www.ebay.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(9,'CNN','http://www.cnn.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(10,'NY Times','http://www.nytimes.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(11,'ESPN','http://espn.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(12,'Amazon','http://www.amazon.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(13,'Weather Channel','http://www.weather.com/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
INSERT INTO "bookmarks" VALUES(14,'BBC','http://www.bbc.co.uk/',0,0,0,NULL,1,NULL,NULL,NULL,NULL);
COMMIT;
sqlite>

Comment 25

[Adrian Tamas (:AdrianT)]

Added the database cleanup in the patch and pushed a new try run:
https://tbpl.mozilla.org/?tree=Try&rev=fd22b4deee27

Comment 26

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v5.1

Still awaiting the run to complete on pandas on the tryserver but it looks good on Tegras now and bookmarks are inserted. There are no other changes in the testImportFromAndroid test from the last green run so we should be ok.

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

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 731857 [details] [diff] [review]
Import bookmarks and history from Android test v5.1

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

lets make the one change, the patch overall is great and so is the try server run.

::: testing/mochitest/runtestsremote.py
@@ +608,5 @@
> +                if test['name'] == "testImportFromAndroid":
> +                    if ("pandaboard" in devOS):
> +                        cmd_del = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser2.db 'delete from bookmarks where title like \"%Bookmark%\"'"]
> +                    else:
> +                        cmd_del = ['execsu', 'sqlite3', "/data/data/com.android.browser/databases/browser.db 'delete from bookmarks where title like \"%Bookmark%\"'"]

since we do this above with:
delete = ['execsu', 'sqlite3', 'delete from bookmarks where _id > 14;']

I would rather do that same command again here vs os specific commands.

Comment 28

[Adrian Tamas (:AdrianT)]

Although there are no errors displayed when doing the first delete I am not 100% sure it works. Doing this at the end instead of the specific OS deletes does not remove the bookmarks from the database. Since we are not specifying the path to the android database for the first command I don't know why the insert after does not work without it but we can't use it at the end to remove the bookmarks.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

oh, I was wrong on my review, we need to have it delete from the specific database.  I assume your try server run had used tegras/pandas that had no previous data on them (i.e. hadn't been run with this test case or other trials/errors before).

Comment 30

[Adrian Tamas (:AdrianT)]

The delete did have an effect although I can't understand why. Before I introduced it the bookmarks were not created on tegras for some reason regardless if it had or didn't have a history from the talos tests.
I'll make the changes for the delete to be OS specific both at the beginning of the test at in the finally section and post a new patch tomorrow.

Comment 31

[Adrian Tamas (:AdrianT)]

For some reason adding the os dependent deletes reintroduces the issue of bookmarks not being inserted on Tegras. Try run: https://tbpl.mozilla.org/?tree=Try&rev=f38a9b6a9023

This is really strange and I can't figure out why a delete that should not work would solve the issue and the proper delete that actually deletes everything but the default bookmarks does not work.

Comment 32

[Adrian Tamas (:AdrianT)]

Please disregard Comment 31. The code was not correct. Made the changes and will post results when the run is complete

Comment 33

[Adrian Tamas (:AdrianT)]

https://tbpl.mozilla.org/?tree=Try&rev=b5c0e9173bc1 

Debug run with print of delete/insert command output print. For some reason on Tegra there is an error opening the database file.

Comment 34

[Adrian Tamas (:AdrianT)]

Joel can you please run this locally on the Tegra and check what is going on? maybe recheck the Tegra database paths since I can only check on phones here.

Comment 35

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v5.2

Comment 36

[Adrian Tamas (:AdrianT)]

Tryserver run for the patch without the print of the database operations: https://tbpl.mozilla.org/?tree=Try&rev=0ff40483814c
Tryserver run with debug print for the database operations:
https://tbpl.mozilla.org/?tree=Try&rev=72ebfd22038f

I am seeing inserts on some tegras but not all of them. What I am seeing consistently on the tegras where the database file is not accessible for the inserts is that the delete from the finally part of the runtestremote script works every time.

Joel can you please investigate this on your local tegra.

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

my local tegra works flawless everytime with this test case.  I am wondering if there is a configuration difference.  Most likely there was a test that ran in the past which has changed the tegra configs so that we get the differing results.

Comment 38

[Adrian Tamas (:AdrianT)]

So how should we continue with this? Should we leave it as it is and hope that once the test has ran on each tegra at least once it will work every time?

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

this will cause the tests to be orange too often.  I am not sure of the best approach here, maybe we could leave them running but not reporting failure on tegras for a week or two, then make a change and turn on checking for tegras.

Comment 40

[Adrian Tamas (:AdrianT)]

It shouldn't be fail on tegras it should just use the default bookmarks from the Android Browser (The 14 default) instead of the default + the 20 added using the sqlite insert

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

I am sort of confused as the what the problem is and what the proposed solution is.  We cannot land anymore tests that increase the orange rate on robocop.

Comment 42

[Adrian Tamas (:AdrianT)]

Attachment: Import bookmarks and history from Android test v5.3

Removed the prints f the sqlite inserts/deletes done in runtestsremote.py.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=f6a80f441430

All fails are existing intermittent fails on other tests. There are no fails on tegras because the bookmarks can't be inserted. In those cases the test is just run with less bookmarks - the default 15 bookmarks on tegras.

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 749141 [details] [diff] [review]
Import bookmarks and history from Android test v5.3

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

while this is fine, the tryserver run is unacceptable for landing new tests.

Comment 44

[Adrian Tamas (:AdrianT)]

Latest tryserver run with the patch: https://tbpl.mozilla.org/?tree=Try&rev=f5f77295ab0e. All fails are existing fails and the run looks green. Can we integrate this?

Comment 45

[Joel Maher ( :jmaher ) (UTC -8)]

this looks much better!

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1c9453a8de

Comment 47

[Ryan VanderMeulen [:RyanVM]]

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