Makoto Kato [:m_kato] Assignee
	Description • 13 years ago

      No description provided.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 13 years ago

This is great stuff! All our migration code needs more tests, this is a good start.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 2 • 13 years ago

should make sure bug 706339 is covered, too :)

	Marco Bonardo [:mak]
	Comment 4 • 13 years ago

I'll review this tomorrow, sorry for late, network bustages didn't help.

As a side note, even doing this kind of testing (that, just to be clear, is absolutely welcome!), we won't be able to catch regressions when other vendors will update their browsers. I suspect we'll always hit cases where handling a failure will be late, since we'll get a bug filed when users are already unable to migrate by some time.
A possible thing we may do, could be to add telemetry for migration failures per browser vendor and keep an eye on it, seeing large increase of failures may mean something changed and we could maybe anticipate fixes.
Thoughts?

	Marco Bonardo [:mak]
	Comment 5 • 13 years ago

Comment on attachment 578454 [details] [diff] [review]
v2

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

Clearing, since this needs another iteration.

I'm mostly thinking of ways to make this harness more modular, especially regarding multiple versions of the same browser.
Issues:
- we may have chrome11 and chrome 24 (guessing numbers) with different formats, and having to test both.
- now that we will add ie, safari and opera, the head will blow up with setup and cleanup methods.

A possible idea I was wondering about may be to do this:
- call tests as test_browserversion_name.js, like test_chrome11_history.js
- have needed data inside a browserversion named folder, like in chrome11/
- inside the data folder have a head.js file doing any setup and cleanup needed
- head_migration.js would extract chrome11 (or whatever name) from the current test name, loadSubscript chrome11/head.js into itself, register data files that are inside chrome11/, and proceed with the test.

So the structure would be like:
chrome11/
  head.js
  History
ie10/
  head.js
  Whatever
head_migration.js
test_chrome11_history.js
test_ie10_history.js

An alternative may be the classic separation like tests/chrome11/, tests/ie10/... with a tests/head_common.js for shared stuff.
what do you think?

::: browser/components/migration/tests/unit/head_migration.js
@@ +56,5 @@
> +  let file = Services.dirsvc.get('ProfD', Ci.nsIFile);
> +  file.append("places.sqlite");
> +  if (file.exists())
> +    file.remove(false);
> +} catch(ex) { dump("Exception: " + ex); }

s/dump/print/

@@ +77,5 @@
> +    },
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIDirectoryServiceProvider]),
> +  });
> +
> +// setup google chrome profile from data

please make this a proper javadoc

@@ +78,5 @@
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIDirectoryServiceProvider]),
> +  });
> +
> +// setup google chrome profile from data
> +function setup_chormeMigration() {

typo: chorme (also in all the call points)

@@ +80,5 @@
> +
> +// setup google chrome profile from data
> +function setup_chormeMigration() {
> +  let fileFrom = do_get_file("data");
> +  let fileTo = fileFrom.clone();

Should this copy to another folder? do_get_file afaict gives a nsIFile pointing to the in-tree content, we should never touch in-tree content for xpcshell-tests convention.
I'd be fine with us making a temporary folder inside gProfD, it will go away magically when the profile is discarded by the harness, rather than forcing us to clean it up. may simplify some of the code here since each test has its own profile.

@@ +93,5 @@
> +    } catch (e) {
> +    }
> +    fileTo.append("Chrome");
> +    fileTo.append("User Data");
> +    fileTo.create(fileTo.DIRECTORY_TYPE, 0755);

octals do warn, but you can use parseInt("0755") to avoid that

@@ +114,5 @@
> +    fileTo.append("google-chrome");
> +    fileTo.create(fileTo.DIRECTORY_TYPE, 0755);
> +  }
> +
> +  fileFrom.copyTo(fileTo, null);

May be safer to use copyToFollowingLinks, I think linux may be using some symlinks (don't remember correctly)

::: browser/components/migration/tests/unit/test_chromeBookmarks.js
@@ +45,5 @@
> +];
> +
> +var gTestCount = 0;
> +
> +let observer = {

nit: please don't mixup let and var

@@ +70,5 @@
> +  {
> +    if (uri == null) {
> +      // ignore for folder
> +      return;
> +    }

well, you may check title for folders, at least.
You can use itemType to differentiate the checks.

@@ +91,5 @@
> +
> +function run_test() {
> +  setup_chormeMigration();
> +  Services.obs.addObserver(observer, "Migration:Ended", false);
> +  PlacesUtils.bookmarks.addObserver(bmObserver, false);

This should be removed somewhere

@@ +99,5 @@
> +  do_check_eq(migrator.sourceExists, true);
> +  do_check_eq(migrator.getMigrateData(null, false) & Ci.nsIBrowserProfileMigrator.BOOKMARKS,
> +              Ci.nsIBrowserProfileMigrator.BOOKMARKS);
> +  migrator.migrate(Ci.nsIBrowserProfileMigrator.BOOKMARKS, null, "");
> +  run_next_test();

run_next_test makes sense when you use add_test, in this case a do_test_pending would make more sense

::: browser/components/migration/tests/unit/test_chromeCookies.js
@@ +53,5 @@
> +
> +    let cs = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
> +
> +    for (let i = 0; i < gTestList.length; i++) {
> +      do_check_eq(cs.getCookieString(NetUtil.newURI(gTestList[i].uri), null, null), gTestList[i].value);

well, also cookies are asynchronous, but in this case I think it may work since they use a synchronous buffer hash and just write asynchronously.

@@ +76,5 @@
> +  do_check_eq(migrator.sourceExists, true);
> +  do_check_eq(migrator.getMigrateData(null, false) & Ci.nsIBrowserProfileMigrator.COOKIES,
> +              Ci.nsIBrowserProfileMigrator.COOKIES);
> +  migrator.migrate(Ci.nsIBrowserProfileMigrator.COOKIES, null, "");
> +  run_next_test();

ditto on do_test_pending

::: browser/components/migration/tests/unit/test_chromeHistory.js
@@ +62,5 @@
> +      let node = root.getChild(i);
> +      do_check_eq(node.uri, gTestList[i].uri);
> +      do_check_eq(node.title, gTestList[i].title);
> +      do_check_eq(node.time, gTestList[i].time);
> +    }

please close the container.

fwiw, you may have used a nsNavHistoryObserver here like you did for bookmarks...
actually I'm not sure this works at all since history addition is asynchronous and doesn't look like you are really waiting for it, the observer approach may be better.

@@ +84,5 @@
> +  do_check_eq(migrator.sourceExists, true);
> +  do_check_eq(migrator.getMigrateData(null, false) & Ci.nsIBrowserProfileMigrator.HISTORY,
> +              Ci.nsIBrowserProfileMigrator.HISTORY);
> +  migrator.migrate(Ci.nsIBrowserProfileMigrator.HISTORY, null, "");
> +  run_next_test();

ditto on using do_test_pending unless you use add_test

::: browser/components/migration/tests/unit/test_profile.js
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

As a side note, in case you don't know, in tests we can use the much shorter pd license http://www.mozilla.org/MPL/boilerplate-1.1/pd-c
it's a more liberal license, so if you don't agree with it you don't have to use it, but it's an option.

@@ +36,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");

Move these to defineLazyModuleGetters in head_migration.js, so that any test will have them in scope.

@@ +40,5 @@
> +Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +
> +
> +function run_test() {
> +  // don't detect chrome since no profile

Adding a multi-line comment at the top of each test file (after the license), explaining the test scope, would be really appreciated.

@@ +41,5 @@
> +
> +
> +function run_test() {
> +  // don't detect chrome since no profile
> +  cleanup_chromeMigration();

fwiw, this should be handled by the head file, not by the test (do_register_cleanup inside setup_chromeMigration() would do that, but as I said I think we may not need cleanup better using folders)

@@ +54,5 @@
> +             createInstance(Ci.nsIBrowserProfileMigrator);
> +
> +  do_check_eq(migrator.sourceExists, true);
> +
> +  do_test_finished();

I don't see a corresponding call to do_test_pending(), so this looks useless.