Let Marionette support shutdown/restart tests

RESOLVED FIXED in mozilla37

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: automatedtester)

Tracking

(Blocks 1 bug, {pi-marionette-client, pi-marionette-server})

Trunk
mozilla37
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [affects=loop] [qa-][qx:P-])

Attachments

(4 attachments, 2 obsolete attachments)

No description provided.
Posted patch WIP v1 (obsolete) — Splinter Review
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
All tests from bug 923606 succeed locally when running:

mach marionette-test browser/components/sessionstore/test/marionette/
After GitHub killed our mirror you can find the "marionette-restart" branch here now:

https://github.com/ttaubert/gecko-dev/commits/marionette-restart
Whiteboard: [triage]
Whiteboard: [triage]
Whiteboard: p=0
Blocks: fxdesktoptriage
No longer blocks: fxdesktopbacklog
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=13
Whiteboard: p=13 → p=13 [qa?]
Assignee: smacleod → nobody
I've wanted this for so many tests. I'll work on this.
Assignee: nobody → gps
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.
Attachment #8443155 - Flags: review?(jgriffin)
Blocks: 1028407
Comment on attachment 8443155 [details] [diff] [review]
Add a restart API to Marionette

I changed this locally. Cancelling review until things are more stable.
Attachment #8443155 - Flags: review?(jgriffin)
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.
I'm planning to add generic restart support first. I'll keep the profile reset case in mind.
Iteration: --- → 33.2
Points: --- → 13
Whiteboard: p=13 [qa?] → [qa?]
We're going to want this for loop as well; we don't need it immediately, but will before too long.
Posted image 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.
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.
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.
(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.
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?
Should be qa- indeed.
Whiteboard: [qa?] → [qa-]
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...
Whiteboard: [qa-] → [qa-][affects=loop]
Depends on: 1033836
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.
needinfo'ing myself
Flags: needinfo?(jgriffin)
Iteration: 33.2 → 33.3
Whiteboard: [qa-][affects=loop] → [affects=loop] [qa-]
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.
Flags: needinfo?(jgriffin)
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?
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.
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.
Depends on: 1035551
Removed from Iteration 33.3.  De-prioritized over other higher priority work contained in the Iteration Priority List.
Status: ASSIGNED → NEW
Iteration: 33.3 → ---
Well, I have nothing assigned to me and I'm nearly done with this. So...
Status: NEW → ASSIGNED
(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.
Dan -- How soon do you need this for Loop tests?
Flags: needinfo?(dmose)
Not immediately; it can wait an iteration or 3.
Flags: needinfo?(dmose)
I'm not working on this, sadly.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Assignee: nobody → dburns
Priority: -- → P1
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 ^^^^^^
Agreed, restarting won't be possible without passing a binary.
: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?
Btw at least for desktop applications you can get the binary location via the following command:

let binary = Services.dirsvc.get("XREExeF", Ci.nsIFile);
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.
Status: NEW → ASSIGNED
(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
(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.
(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.
Attachment #8522915 - Flags: review?(jgriffin)
/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 6002be1c422575feaa5f485267f929aa66f0acf8
/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 6002be1c422575feaa5f485267f929aa66f0acf8
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.
Attachment #8335234 - Attachment is obsolete: true
/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
Attachment #8522915 - Flags: review?(jgriffin) → review+
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.
Flags: needinfo?(dburns)
This shouldnt have caused errors like this so will redo try
Flags: needinfo?(dburns)
I appear to have broken something in gaiatest and am struggling to get this working locally so that I can debug properly
Attachment #8522915 - Flags: review?(jgriffin)
Attachment #8522915 - Flags: review+
I have updated MozReview in https://reviewboard.mozilla.org/r/633/ with the changes to make it work everywhere.
/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 9deb1c57df1c493aa0c29ece5c6cc2ae6e472d4c
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
Note: I recommend another try run with -p linux64_gecko -u all to make sure we haven't regressed gaiatest again.
Attachment #8522915 - Flags: review?(jgriffin) → review+
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.
I have no idea how no gecko changes can force a gecko error...
gaiatest restarts the binary often; perhaps this patch changes the way these restarts work with the profile in some way
Attachment #8522915 - Flags: review+ → review?(jgriffin)
/r/633 - Bug 940954: Allow marionette to restart the browser and create a new session

Pull down this commit:

hg pull review -r 536b85e5e82e12eb7710135945217dfe707e5b5d
Attachment #8522915 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/e11a7e1b0477
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Iteration: --- → 37.1
Attachment #8522915 - Attachment is obsolete: true
Attachment #8618059 - Flags: review+
Attachment #8618060 - Flags: review+
Whiteboard: [affects=loop] [qa-] → [affects=loop] [qa-][qx:P-]
You need to log in before you can comment on or make changes to this bug.