940954 - Let Marionette support shutdown/restart tests

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P1)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: WIP v1

The whole WIP in a single patch.

A more detailed set of patches with verbose commit messages can be found in my 'marionette-restart' branch:

https://github.com/ttaubert/mozilla-central/commits/marionette-restart

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

All tests from bug 923606 succeed locally when running:

mach marionette-test browser/components/sessionstore/test/marionette/

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

After GitHub killed our mirror you can find the "marionette-restart" branch here now:

https://github.com/ttaubert/gecko-dev/commits/marionette-restart

Comment 4

[Gregory Szorc [:gps]]

I've wanted this for so many tests. I'll work on this.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Add a restart API to Marionette

Marionette instances can now be restarted by calling a restart()
method. This will shut down the GeckoInstance and start it up again
using the same options and same profile. Restarts can be initiated
either inside the application or outside. This means the restart
API can be both Python and JavaScript tests.

Support for restarting the emulator is not currently implemented. This
was a rather arbitrary decision based on the relative complexity of the
emulator code compared to GeckoInstance.

An API to trigger restarts via JavaScript will come in subsequent
patches.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 8443155 [details] [diff] [review]
Add a restart API to Marionette

I changed this locally. Cancelling review until things are more stable.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Restart capabilities for Marionette will also be a dependency for us to transition Mozmill over to Marionette. We already have this capability with Mozmill, so if you need feedback or help please let me know. The code in Mozmill is very complicated, so we may not be able to take anything of it. But at least we can check if we care of all the states here. During the next quarter we want to stabilize Mozmill restart capabilities even more, so that we could compare the results between Mozmill and Marionette.

Something this patch or a follow-up might want to add, is that the profile can be reset. In some cases it is more helpful as resetting dozen of different prefs, and caches.

Comment 8

[Gregory Szorc [:gps]]

I'm planning to add generic restart support first. I'll keep the profile reset case in mind.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

We're going to want this for loop as well; we don't need it immediately, but will before too long.

Comment 10

[Gregory Szorc [:gps]]

Attachment: sketch of proposal

Just wanted to throw up my sketch of how I plan to implement this.

Essentially, there will be a low-level Marionette API to trigger restart. When that's called, the Marionette server sends an out-of-band message to the Marionette client saying a restart was requested. Contained are details like "is this a quit" and "is this a restart." Over time, we can add things like "please reset profile."

When Marionette tests are executing, the test runner will look for these out-of-band messages after each script execution. If a shutdown message was received, we restart the application and execute the script again.

This is where things get a little complicated - and this isn't captured on the diagram.

I want to introduce a JS API for running tests that restart. Instead of a test() or testGenerator() special function, we'll have a testWithRestarts() special function (name bikeshedding welcome). This function will be a generator of functions. Each function inside the generator will get called on its appropriate application invocation. e.g.

function* testWithRestarts(ctx) {
  yield (ctx) => {
    // Do stuff on initial run.
    ctx.restart();
  };

  yield (ctx) => {
    // Do stuff on second run.
    ctx.restart();
  };

  yield (ctx) => {
    // Do stuff on third run.
    ctx.finish();
  };
}

Under the hood, the server and client are exchanging a "test function offset" parameter as part of the out-of-band restart message. The thing that calls testWithRestarts() will .next() over the generator until it gets to the function appropriate for that process invocation.

This approach should result in procedural looking tests that perform restarts. It doesn't require individual tests to maintain any state about how many restarts have been performed, etc.

Note: this solution may preclude tests that need to restart an indeterminate number of times. I'm not sure if we have any tests like that. Worse case, these tests can build upon the low-level APIs and have a custom test() function that looks for custom arguments and does its own dispatching depending on state.

Comment 11

[Jonathan Griffin (:jgriffin)]

Nice design.  I look forward to seeing it.

Presumably the runner will save the state of assertion methods called between restarts, so for example, we fail the test if the only assertion failure occurred prior to the first restart.

Comment 12

[Gregory Szorc [:gps]]

Correct. The test runner will buffer the results objects until completion or failure. They are just counts AFAICT, so we can sum them at the end. If the result objects ever get more complicated, this solution starts to fall apart. We'll cross that bridge if we get to it, I suppose.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Gregory Szorc [:gps] from comment #10)
> Essentially, there will be a low-level Marionette API to trigger restart.
> When that's called, the Marionette server sends an out-of-band message to
> the Marionette client saying a restart was requested. Contained are details
> like "is this a quit" and "is this a restart." Over time, we can add things
> like "please reset profile."

Restart in safe-mode would be another thing we need.

> Under the hood, the server and client are exchanging a "test function
> offset" parameter as part of the out-of-band restart message. The thing that
> calls testWithRestarts() will .next() over the generator until it gets to
> the function appropriate for that process invocation.

Calling .next() until we're at the right position sounds bad, that would execute all that code before again, no?

The approach I took in my WIP patch (attachment 8335234 [details] [diff] [review]) is that an iteration number is passed to marionette.execute_js_script() so that this number is available as a marionette param:

> let {iteration} = this.__marionetteParams[0];

Like the WIP patch in attachment 8335231 [details] [diff] [review] the test suite could then decide to skip |iteration| number of tasks. A little wrapper for add_task() called add_iteration_task() makes it easy to skip the task definition if the number of tasks to ignore isn't reached yet.

Comment 14

[Robert Kaiser]

This is marked as [qa?] but I wonder what QA would be able to test here. Am I right there's nothing that we'd do in manual QA here and that all the testing for this will be by actually having automated tests use the functionality added here?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Should be qa- indeed.

Comment 16

[Gregory Szorc [:gps]]

In the spirit of full disclosure, I'm actively working on this... but only when the Firefox update hotfix (tracked in bug 928173) isn't capturing my attention. Bugs in it take priority over this. And quite a few were filed in the past few days...

Comment 17

[Gregory Szorc [:gps]]

OK, I've got basic restartable tests working!

I still need to work out some issues with logging and result capture. But the basic framework is in place.

Patch series begins at 
http://hg.gregoryszorc.com/gecko-collab/rev/a1e6c5b59e32

Here's what a restarting test currently looks like:

http://hg.gregoryszorc.com/gecko-collab/file/339a53cac414/testing/marionette/client/marionette/tests/unit/test_restart_single.js

I abused this opportunity to add a JSM for defining test declaration and running primitives. I may or may not tack on add_test/add_task APIs while I'm here :)

jgriffin: I wouldn't mind your preliminary feedback on the patch set. But I understand if you don't want to sort through raw diffs on a Mercurial server. I can upload the patches if you want.

Comment 18

[Jonathan Griffin (:jgriffin)]

needinfo'ing myself

Comment 19

[Gregory Szorc [:gps]]

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

Comment 20

[Jonathan Griffin (:jgriffin)]

Nice, I like it.  I didn't know it was that easy to register resource:// files at runtime; we can probably make more use of this.

For the restart() method, we'll probably want to check that self.instance is not None.  It's possible to run Marionette tests against a browser you've started yourself, in which case self.instance = None, and we can't honor restart requests in such a case.

Comment 21

[Gregory Szorc [:gps]]

The hardest part about adding resource:// mappings is reconfiguring automation. We'll need to add a command line argument to define the testing modules path. This means we need to update mozharness to add the command line. But if mozharness passes this command line to a Marionette that doesn't recognize it, argparse will reject it.

AFAICT, we don't have an in-tree mechanism of defining which mozharness commit to use. So deploying this is going to be... fun. Scenarios like this are why I want most of our automation code to live in the tree or for the tree to be linked to a specific commit of the automation code to run.

But things have been changing a lot recently. Maybe my knowledge is out of date?

Comment 22

[Gregory Szorc [:gps]]

Err, I guess mozharness has the in-tree config files for adding extra options. Hopefully I can coerce this into working. My knowledge is mostly from the pre-mozharness era.

Comment 23

[Jonathan Griffin (:jgriffin)]

We have a better story for this than we used to.

Basically, we want to add in-tree mozharness config files for Marionette, such as we've done for other unit tests, here:  http://mxr.mozilla.org/mozilla-central/source/testing/config/mozharness/

Then, we can define per-tree command-line args and still use the same mozharness script overall.

Comment 24

[Marco Mucci [:MarcoM]]

Removed from Iteration 33.3.  De-prioritized over other higher priority work contained in the Iteration Priority List.

Comment 25

[Gregory Szorc [:gps]]

Well, I have nothing assigned to me and I'm nearly done with this. So...

Comment 26

[Marco Mucci [:MarcoM]]

(In reply to Gregory Szorc [:gps] from comment #25)
> Well, I have nothing assigned to me and I'm nearly done with this. So...

Please refer to Dolske's email regarding the available work on the Iteration Priority List which is important for this iteration.

Comment 27

[Maire Reavy [:mreavy]]

Dan -- How soon do you need this for Loop tests?

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Not immediately; it can wait an iteration or 3.

Comment 29

[Gregory Szorc [:gps]]

I'm not working on this, sadly.

Comment 30

[David Burns :automatedtester]

Looking at how the runner will handle this I am thinking that I am going to have to mandate that restart tests have had a binary passed through. If you are connecting to an already started browser then you won't be able to do restarts mainly because we don't have a mechanism for getting the binary location.

If you oppose this please speak up else I will speak it to hanlde the ^^^^^^

Comment 31

[Jonathan Griffin (:jgriffin)]

Agreed, restarting won't be possible without passing a binary.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

:AutomatedTester I'm not sure how to parse your question.  It sounds like this is something that would effect how tests restartable tests are structure.  If that's true, who would pass what binary to whom when?

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Btw at least for desktop applications you can get the binary location via the following command:

let binary = Services.dirsvc.get("XREExeF", Ci.nsIFile);

Comment 34

[David Burns :automatedtester]

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

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

David, please be aware that in case for Mozmill tests we also need a restart triggered by an action inside of Firefox, e.g. restart button after installing an add-on. If you think that this should be moved out to a new bug please let me know and I can file one with out specific needs.

Comment 36

[David Burns :automatedtester]

(In reply to Dan Mosedale (:dmose) - not reading bugmail; needinfo? for response from comment #32)
> :AutomatedTester I'm not sure how to parse your question.  It sounds like
> this is something that would effect how tests restartable tests are
> structure.  If that's true, who would pass what binary to whom when?

The --binary flag would be needed when running tests pointing at the binary under test. ./mach marionette-test takes care of this so shouldnt affect you. It's more you will get an exception if you call marionette.restart() and there is no binary location passed in

Comment 37

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Btw at least for desktop applications you can get the binary location via
> the following command:
> 
> let binary = Services.dirsvc.get("XREExeF", Ci.nsIFile);

doing something like this will require a lot of work for not much value, or at least I can't see it, since you can handle with the test runner by passing in --binary.

Comment 38

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #35)
> David, please be aware that in case for Mozmill tests we also need a restart
> triggered by an action inside of Firefox, e.g. restart button after
> installing an add-on. If you think that this should be moved out to a new
> bug please let me know and I can file one with out specific needs.

I suggest raising a new bug for this, I think it should be trivial but I would rather make sure we handle your use cases properly.

Comment 39

[David Burns :automatedtester]

Attachment: MozReview Request: bz://940954/AutomatedTester

Comment 40

[David Burns :automatedtester]

/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 6002be1c422575feaa5f485267f929aa66f0acf8

Comment 41

[David Burns :automatedtester]

/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 6002be1c422575feaa5f485267f929aa66f0acf8

Comment 42

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/631/#review293

::: testing/marionette/client/marionette/marionette.py
(Diff revision 1)
> +    def restart(self):

Can we merge restart and restart_with_clean_profile, and have clean_profile a kwarg?  This shouldn't cause any gaiatest dependency problems, as this method isn't used in gaiatest.

::: testing/marionette/client/marionette/tests/unit/test_profile_management.py
(Diff revision 1)
> +        self.marionette.enforce_gecko_prefs({"marionette.test.bool": True,

Might as well remove the prefs that aren't actually used in the test.

Comment 43

[David Burns :automatedtester]

/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session
/r/705 - updated after review comments

Pull down these commits:

hg pull review -r 0bc773a209227fa8b580377eeedddcceab13705a

Comment 44

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/705/#review331

Looks good, thanks!

Comment 45

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/631/#review333

Ship It!

Comment 46

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

Originally landed with the wrong bug number (bug 940955) in https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9fc64e82a8
Backed that out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c79206426b84
Relanded DONTBUILD in https://hg.mozilla.org/integration/mozilla-inbound/rev/e9029cbd137c with the correct bug number.

Comment 47

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

And backed out again for Gip test failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c620bc5082c

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3970554&repo=mozilla-inbound

16:24:22 INFO - Results will not be posted to Treeherder. Please set the following environment variables to enable Treeherder reports: TREEHERDER_KEY, TREEHERDER_SECRET
16:30:24 INFO - Traceback (most recent call last):
16:30:24 INFO - File "/builds/slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/cli.py", line 4, in <module>
16:30:24 INFO - main()
16:30:24 INFO - File "/builds/slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/runtests.py", line 106, in main
16:30:24 INFO - cli(runner_class=GaiaTestRunner, parser_class=GaiaTestOptions)
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runtests.py", line 35, in cli
16:30:24 INFO - runner = startTestRunner(runner_class, options, tests)
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runtests.py", line 20, in startTestRunner
16:30:24 INFO - runner.run_tests(tests)
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runner/base.py", line 703, in run_tests
16:30:24 INFO - self.capabilities
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/runner/base.py", line 547, in capabilities
16:30:24 INFO - self.marionette.start_session()
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 827, in start_session
16:30:24 INFO - self.session = self._send_message('newSession', 'value', capabilities=desired_capabilities, session_id=session_id)
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/decorators.py", line 36, in _
16:30:24 INFO - return func(*args, **kwargs)
16:30:24 INFO - File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/marionette/marionette.py", line 628, in _send_message
16:30:24 INFO - "Connection timed out", status=errors.ErrorCodes.TIMEOUT)
16:30:24 INFO - marionette.errors.TimeoutException: TimeoutException: Connection timed out
16:30:24 ERROR - Return code: 1
16:30:24 INFO - gecko.log not found
16:30:24 INFO - TinderboxPrint: marionette: <em class="testfail">T-FAIL</em>
16:30:24 ERROR - Marionette exited with return code 1: harness failures
16:30:24 ERROR - # TBPL FAILURE #
16:30:24 INFO - Running post-action listener: _resource_record_post_action
16:30:24 INFO - Running post-run listener: _resource_record_post_run
16:30:25 INFO - Total resource usage - Wall time: 474s; CPU: 25.0%; Read bytes: 159793152; Write bytes: 447287296; Read time: 8624; Write time: 474648
16:30:25 INFO - install - Wall time: 37s; CPU: 100.0%; Read bytes: 4096; Write bytes: 100982784; Read time: 8; Write time: 129880
16:30:25 INFO - run-marionette - Wall time: 438s; CPU: 18.0%; Read bytes: 159027200; Write bytes: 346304512; Read time: 8576; Write time: 344768
16:30:25 INFO - Running post-run listener: _upload_blobber_files
16:30:25 INFO - Blob upload gear active.

Comment 48

[David Burns :automatedtester]

This shouldnt have caused errors like this so will redo try

Comment 49

[David Burns :automatedtester]

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

Comment 50

[David Burns :automatedtester]

I appear to have broken something in gaiatest and am struggling to get this working locally so that I can debug properly

Comment 51

[David Burns :automatedtester]

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

Comment 52

[David Burns :automatedtester]

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

Comment 53

[David Burns :automatedtester]

I have updated MozReview in https://reviewboard.mozilla.org/r/633/ with the changes to make it work everywhere.

Comment 54

[David Burns :automatedtester]

/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 9deb1c57df1c493aa0c29ece5c6cc2ae6e472d4c

Comment 55

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/633/#review645

::: testing/marionette/client/marionette/geckoinstance.py
(Diff revision 2)
> -                  gecko_log=None, prefs=None):
> +                  gecko_log=None, prefs=None, ):

nit: remove trailing comma

Comment 56

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/633/#review647

Ship It!

Comment 57

[Jonathan Griffin (:jgriffin)]

Note: I recommend another try run with -p linux64_gecko -u all to make sure we haven't regressed gaiatest again.

Comment 58

[David Burns :automatedtester]

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

Comment 59

[David Burns :automatedtester]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b87e7511983

Comment 60

[Phil Ringnalda (:philor)]

I recommend Mac b2g desktop.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8fb3fdb421 to see whether this was the reason we started getting sqlite failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4342125&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=4339929&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=4339930&repo=mozilla-inbound starting with this push, and not happening on the push before despite 50-60 retriggers.

Comment 61

[David Burns :automatedtester]

I have no idea how no gecko changes can force a gecko error...

Comment 62

[Jonathan Griffin (:jgriffin)]

gaiatest restarts the binary often; perhaps this patch changes the way these restarts work with the profile in some way

Comment 63

[David Burns :automatedtester]

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

Comment 64

[David Burns :automatedtester]

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

Comment 65

[David Burns :automatedtester]

/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 536b85e5e82e12eb7710135945217dfe707e5b5d

Comment 66

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/633/#review677

Ship It!

Comment 67

[David Burns :automatedtester]

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

Comment 68

[Ryan VanderMeulen [:RyanVM]]

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

Comment 70

[David Burns :automatedtester]

Attachment: MozReview Request: Bug 940954: Allow marionette to restart the browser and create a new session

Comment 71

[David Burns :automatedtester]

Attachment: MozReview Request: updated after review comments

Comment 72

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Moving bugs for Marionette client due to component changes.