888624 - Add tests for session migration

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P5)

Description

[:Gijs (he/him)]

There should be some automated tests for migrating sessions during Firefox reset.

Comment 1

[:Gijs (he/him)]

This should be possible post- bug 940954, so I'm putting this in the backlog.

Comment 2

[:Gijs (he/him)]

Matt, I'm looking at this and actually have at least started grokking the marionette docs and set-up procedure. I have something of a no-op tests that creates a bookmark, a login object, a preference, and changes the homepage, and that verifies that these settings exist in that profile. I then intend to make the marionette test restart with the relevant reset parameters as commandline arguments, and then run those verifications again (where everything but the added pref should still be there). I can then also add checks for the other things we're supposed to be migrating.

The problem I'm facing is that when I checked how to ensure we're running reset/refresh, it seems the implementation requires that the profile we're resetting is the default profile. That requires messing with the profiles.ini file and so on, AIUI, to make the test profile that marionette runs with the default profile. Does this sound correct to you? Is there an alternative we can use that wouldn't require changing profiles.ini ?

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> it seems the implementation requires that the profile we're
> resetting is the default profile. That requires messing with the
> profiles.ini file and so on, AIUI, to make the test profile that marionette
> runs with the default profile. Does this sound correct to you? Is there an
> alternative we can use that wouldn't require changing profiles.ini ?

Specifically, I'm thinking of:

https://dxr.mozilla.org/mozilla-central/rev/a235bfcc8c411169b82420c503775c1a3e7edad5/toolkit/xre/nsAppRunner.cpp#2339-2349

Comment 4

[Matthew N. [:MattN]]

In case you didn't notice, I believe you can assign to the toolkit profile service `selectedProfile` attribute and then `flush()` so you don't need to manually manipulate profiles.ini. Can you run arbitrary JS code to do this via Marionette in the original profile? If so, I think that's probably easiest for now unless we want to reverse our decision mentioned below. I'm not sure if you would need to handle restoring the default profile after (to avoid breaking other tests/code) but that shouldn't be too hard assuming you can manipulate the profile service already.

Example flow:
1) In profile A, get the default profile: nsIToolkitProfileService.defaultProfile.name and store this in the marionette process as a string
2) Set profile A as the default
3) Run the reset
4) Restore the default profile stored in (1) using getProfileByName + nsIToolkitProfileService.defaultProfile

https://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsIToolkitProfileService.idl#28

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > it seems the implementation requires that the profile we're
> > resetting is the default profile. That requires messing with the
> > profiles.ini file and so on, AIUI, to make the test profile that marionette
> > runs with the default profile. Does this sound correct to you? Is there an
> > alternative we can use that wouldn't require changing profiles.ini ?
> 
> Specifically, I'm thinking of:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> a235bfcc8c411169b82420c503775c1a3e7edad5/toolkit/xre/nsAppRunner.cpp#2339-
> 2349

Yeah, this is burning us again… When I initially implemented this I had the browser set an environment variable with the profile name/path to tell the new instance of Fx which profile to reset. Mano didn't like this so we put in this weird restriction that it only works with the default profile. I regret not pushing back on that more but one argument was that we don't "support" multiple profiles (yet?).

Comment 5

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #4)
> In case you didn't notice, I believe you can assign to the toolkit profile
> service `selectedProfile` attribute and then `flush()` so you don't need to
> manually manipulate profiles.ini. Can you run arbitrary JS code to do this
> via Marionette in the original profile?

Yes, though see below. It wasn't so much about "how do I technically do it", and more about "um, do we want to press ahead doing it this way, because it seems hacky / not very safe / a bit silly.

> If so, I think that's probably
> easiest for now unless we want to reverse our decision mentioned below.

Well, when I started as an employee, I seem to remember one of the first things I had to deal with was having the backups we make not overwrite one another. That (ie would we clobber data if you reset several profiles) is the only reason I can think of not to do this. And in any case, the current restriction doesn't really prevent you from selecting one profile after another...

It also doesn't seem like a restriction that folks from UX or something would actually value... but then, I don't know if I want to bother shaving this particular yak. :-\

> I'm
> not sure if you would need to handle restoring the default profile after (to
> avoid breaking other tests/code) but that shouldn't be too hard assuming you
> can manipulate the profile service already.

This is assuming that the test works reliably and the new copy of Firefox comes up normally. I wouldn't much like to leave it in a state where we've made the default profile some temporary marionette profile and then bust up tests on that machine later or something... Anyway, I can probably make a backup of the file contents from the python and always restore them afterwards, or something.

Comment 6

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #4)
> In case you didn't notice, I believe you can assign to the toolkit profile
> service `selectedProfile` attribute and then `flush()` so you don't need to
> manually manipulate profiles.ini. Can you run arbitrary JS code to do this
> via Marionette in the original profile? If so, I think that's probably
> easiest for now unless we want to reverse our decision mentioned below. I'm
> not sure if you would need to handle restoring the default profile after (to
> avoid breaking other tests/code) but that shouldn't be too hard assuming you
> can manipulate the profile service already.
> 
> Example flow:
> 1) In profile A, get the default profile:
> nsIToolkitProfileService.defaultProfile.name and store this in the
> marionette process as a string
> 2) Set profile A as the default

Except that this bit doesn't work. It looks like you can't set the defaultProfile to the selectedProfile if the selectedProfile is not in the profile list to begin with (ie isn't in the profiles/ directory). There are no errors, the setter just seems to do nothing... :-\

Comment 7

[:Gijs (he/him)]

Oh, no, sorry, the problem is different: profileService.selectedProfile just lies and gives you the default profile instead of the current one. Easy to test with ./mach run without passing it a profile (in which case it'll use a temp one in your objdir). :-\

Comment 8

[:Gijs (he/him)]

Attachment: add test for Firefox reset, f?MattN

So this doesn't work. And that's kind of surprising to me. I added a bunch of debug logging and I can't figure out why. The files that get copied to the backup directory on my desktop are fine and still have my changes (e.g. the change of homepage). So I don't think it's clobbering those. But the new profile gets none of that, I see none of that in about:config or about:preferences (I've made it wait when it finishes in this WIP copy). Any ideas, Matt? I'm a bit lost. It doesn't help that it seems to be very difficult to get decent debugging output from marionette. Maybe the XRE_PROFILE_PATH thing is breaking it somehow? Don't really know how - I can't do without it though, because the mozrunner instance marionette use forces --profile path/to/profile into the command arguments, and so then we don't do a reset. Thoughts?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8739195 [details] [diff] [review]
add test for Firefox reset, f?MattN

I figured out a way around the problem here. Seems to be caused by marionette just killing the process.

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

Review commit: https://reviewboard.mozilla.org/r/45311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45311/

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> Created attachment 8739553 [details]
> MozReview Request: Bug 888624 - add test for Firefox reset, r?MattN
> 
> Review commit: https://reviewboard.mozilla.org/r/45311/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/45311/

This is pretty rough, and we're not checking everything yet (notably: we're not checking session restore functionality yet) but this is already big enough that I figured it was worth checking in what you think about the approach. If this looks reasonable to you, I figure we can trypush and land this, and then iterate. Because of the restarts and the messing about with profiles.ini it's a fairly scary test.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

https://reviewboard.mozilla.org/r/45311/#review43191

f+. I didn't look closely at the profile / env. var stuff as I believe you're going to make the Refresh feature work with non-default profiles.

You should probably get review from a marionette peer on the stuff related to marionette startup.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:105
(Diff revision 1)
> +  def checkPassword(self):
> +    loginInfo = self.marionette.execute_script("""
> +      let ary = Services.logins.findLogins({}, 

How about also checking the number of saved logins? See getAllLogins()

Same for form history and other data types that are unlikely to have break from pre-loaded data.

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:107
(Diff revision 1)
> +                           true, false, false, expireTime);
> +    """, script_args=[self._cookieHost, self._cookiePath, self._cookieName, self._cookieValue])
> +
> +  def checkPassword(self):
> +    loginInfo = self.marionette.execute_script("""
> +      let ary = Services.logins.findLogins({}, 

Nit: trailing whitespace

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:120
(Diff revision 1)
> +
> +  def checkBookmark(self):
> +    titleInBookmarks = self.marionette.execute_script("""
> +      let url = arguments[0];
> +      let bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(makeURI(url), {}, {});
> +      return bookmarkIds.length ? PlacesUtils.bookmarks.getItemTitle(bookmarkIds[0]) : "";

e.g. how about making the condition `.length == 1` so multiple matches won't pass the tests

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:200
(Diff revision 1)
> +  def checkSession(self):
> +    pass

Add a bug number for a follow-up?

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:223
(Diff revision 1)
> +      global.profSvc = Components.classes["@mozilla.org/toolkit/profile-service;1"]
> +                       .getService(Components.interfaces.nsIToolkitProfileService);
> +      global.Preferences = Cu.import("resource://gre/modules/Preferences.jsm", {}).Preferences;

Nit: You're using Cu but not Cc and Ci. Use the alias for all 3?

::: testing/marionette/components/marionettecomponent.js:100
(Diff revision 1)
>    if (cmdLine.handleFlag("marionette", false)) {
> +    dump("Got marionette flag");
> +    Cu.reportError("Got marionette flag");

Leftover?

Comment 13

[:Gijs (he/him)]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45311/diff/1-2/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45311/diff/2-3/

Comment 15

[David Burns :automatedtester]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

https://reviewboard.mozilla.org/r/45311/#review48173

I havent reviewed the tests themselves but everything else is fine.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

https://reviewboard.mozilla.org/r/45311/#review48847

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:114
(Diff revisions 1 - 3)
> +                                 Ci.nsIWebProgressListener.STATE_IS_NETWORK;
> +          let {TabStateFlusher} = Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {});
> +          let expectedURLs = Array.from(arguments[0])
> +          gBrowser.addTabsProgressListener({
> +            onStateChange(browser, webprogress, request, flags, status) {
> +              Cu.reportError("statechange");

Nit: leftover?

::: browser/components/migration/tests/marionette/test_refresh_firefox.py:119
(Diff revisions 1 - 3)
> +              Cu.reportError("statechange");
> +              try {
> +                request && request.QueryInterface(Ci.nsIChannel);
> +              } catch (ex) {}
> +              let uriLoaded = request.originalURI && request.originalURI.spec;
> +              Cu.reportError("statechange, uri: " + uriLoaded);

Ditto

::: testing/marionette/components/marionettecomponent.js:147
(Diff revision 3)
> +MarionetteComponent.prototype.maybeReadPrefsFromEnvironment = function() {
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  if (env.exists(ENV_PREF_VAR)) {
> +    let prefStr = env.get(ENV_PREF_VAR);

Nit: a JSDoc comment about the purpose and format of the env. var. would be good.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45311/diff/3-4/

Comment 18

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e35efceeb067

Comment 19

[:Gijs (he/him)]

Looking at logs for the trypush, I don't see my test running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e35efceeb067&selectedJob=20710052

Should I be using different jobs? Which ones? Have I just made a mistake wrt the manifest that lists the test, or the moz.build file that adds the manifest?

Comment 20

[David Burns :automatedtester]

We are going to need to add it to  https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/unit-tests.ini for the mean time.

Comment 21

[:Gijs (he/him)]

(In reply to David Burns :automatedtester from comment #20)
> We are going to need to add it to 
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/
> marionette/tests/unit-tests.ini for the mean time.

Thanks!

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db8c81d30b9

Comment 22

[:Gijs (he/him)]

Comment on attachment 8739553 [details]
MozReview Request: Bug 888624 - add test for Firefox refresh, r=MattN,AutomatedTester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45311/diff/4-5/

Comment 23

[David Burns :automatedtester]

(orthogonal to getting this running on automation I am getting a failing test for this )

6:36.15 TEST_START: MainThread test_refresh_firefox.py TestFirefoxRefresh.testReset
 7:36.75 LOG: MainThread WARNING Failed to gather test failure debug.
Traceback (most recent call last):

  File "/Users/dburns/development/mozilla-central/testing/marionette/harness/marionette/runner/base.py", line 611, in gather_debug
    with marionette.using_context(marionette.CONTEXT_CHROME):

  File "/usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1366, in using_context
    scope = self._send_message("getContext", key="value")

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 727, in _send_message
    msg = self.client.request(name, params)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py", line 291, in request
    self.send(cmd)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py", line 271, in send
    raise e

error: [Errno 9] Bad file descriptor

 7:36.75 TEST_END: MainThread ERROR, expected PASS
Traceback (most recent call last):
  File "/Users/dburns/development/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 351, in run
    testMethod()
  File "/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 404, in testReset
    self.doReset()
  File "/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 374, in doReset
    self.marionette.restart(clean=False, in_app=True)
  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1134, in restart
    self.raise_for_port(self.wait_for_port())
  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)
  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 689, in raise_for_port
    raise IOError("Timed out waiting for port!")
IOError: Timed out waiting for port!
 7:36.75 LOG: MainThread WARNING Failed to gather test failure debug.
Traceback (most recent call last):

  File "/Users/dburns/development/mozilla-central/testing/marionette/harness/marionette/runner/base.py", line 611, in gather_debug
    with marionette.using_context(marionette.CONTEXT_CHROME):

  File "/usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1366, in using_context
    scope = self._send_message("getContext", key="value")

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 36, in _
    return func(*args, **kwargs)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 727, in _send_message
    msg = self.client.request(name, params)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py", line 291, in request
    self.send(cmd)

  File "/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py", line 271, in send
    raise e

error: [Errno 9] Bad file descriptor

 7:36.75 LOG: MainThread ERROR test_end for test_refresh_firefox.py TestFirefoxRefresh.testReset logged while not in progress. Logged with data: {"status": "ERROR", "extra": {"class_name": "test_refresh_firefox.TestFirefoxRefresh", "method_name": "testReset"}, "expected": "PASS", "test": "test_refresh_firefox.py TestFirefoxRefresh.testReset", "message": "error: [Errno 9] Bad file descriptor", "stack": "Traceback (most recent call last):\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/harness/marionette/marionette_test.py\", line 381, in run\n    self.tearDown()\n  File \"/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py\", line 316, in tearDown\n    self.marionette.restart(clean=True, in_app=False)\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py\", line 1132, in restart\n    self.delete_session()\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py\", line 1194, in delete_session\n    self._send_message(\"deleteSession\")\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/decorators.py\", line 36, in _\n    return func(*args, **kwargs)\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/marionette.py\", line 727, in _send_message\n    msg = self.client.request(name, params)\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py\", line 291, in request\n    self.send(cmd)\n  File \"/Users/dburns/development/mozilla-central/testing/marionette/client/marionette_driver/transport.py\", line 271, in send\n    raise e\n"}
 7:36.75 LOG: MainThread INFO START LOG:
 7:36.75 LOG: MainThread INFO Error getting log: [Errno 9] Bad file descriptor
 7:36.75 LOG: MainThread INFO END LOG:
 7:36.75 LOG: MainThread INFO

Comment 24

[David Burns :automatedtester]

so it looks like I needed to rebuild after speaking to :gijs. Getting a new error

6:42.23 TEST_START: MainThread test_refresh_firefox.py TestFirefoxRefresh.testReset
 6:54.62 TEST_END: MainThread FAIL, expected PASS
Traceback (most recent call last):
  File "/Users/dburns/development/mozilla-central/testing/marionette/harness/marionette/marionette_test.py", line 351, in run
    testMethod()
  File "/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 407, in testReset
    self.checkProfile(hasMigrated=True)
  File "/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 278, in checkProfile
    self.checkSession()
  File "/Users/dburns/development/mozilla-central/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 268, in checkSession
    self.assertSequenceEqual(tabURIs, ["about:blank"] + self._expectedURLs)
AssertionError: Sequences differ: [u'about:blank', u'about:blank... != ['about:blank', 'about:robots'...

First differing element 1:
about:blank
about:robots

- [u'about:blank', u'about:blank', u'about:blank']
?  -               -        ^^^^   -       ^  --

+ ['about:blank', 'about:robots', 'about:mozilla']
?                        ++ ^^^          ^^^^^
 6:58.81 LOG: MainThread INFO

Comment 25

[David Burns :automatedtester]

Chris,

I am stuck to why this test is not being picked up by automation, everything appears to be in place and it does work locally so am confused. Since you have done test based work, can you spot anything ?

Comment 26

[Chris Manchester (limited bugmail, email directly)]

In comment 21 the added test is skipped (filtered on type 'browser'), I think due to putting "browser = True" in the manifest when the harness expects "browser = true", which is fixed in the most recent reviewboard revision.

Comment 27

[:Gijs (he/him)]

Chris: *thanks* !

(I filed bug 1272414 and put up a patch there to stop some other poor soul from hitting this.)

Let's try that again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5f6c2239440a238c65082477ac7846f00e3d106

(based on comment #24, also going to test locally on OS X, but it seems the backlog on try is such that it's not much use pushing it there...)

Comment 28

[:Gijs (he/him)]

(I'll land this my tomorrow morning but I want to be around in case there's issues - pushing to try to make sure windows and osx pass feels pointless because things won't run before tomorrow anyway...)

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b726536ea4d

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b726536ea4d