857599 - Clone profile for Marionette tests to prevent polluting the original profile

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Cloning profiles was introduced in bug 642843 but currently mozrunner does not allow you to specify that you wish to clone a profile. Please correct me if I'm wrong.

We are running tests on the B2G desktop build and a prebuilt profile. The profile should be reset between tests, and I have been able to successfully test this by adding a call to Profile.clone(profile.profile) in mozrunner, but I feel this would be best suited to profile_args. I would even propose that we clone by default.

Comment 1

[Jeff Hammel]

Assuming this is x-platform

Comment 2

[Jeff Hammel]

So this *should* be possible with zero code changes (!).

If you subclass the runner and change the `profile_class` attribute to e.g. Profile.clone *AND* pass the path_from as part of profile_args to the mozrunner create() method, ABICT this *should* work.  I have not tested.

Assuming that this is so and fits your need, IMHO we should change profile_class to profile_factory (a class being an instance factory and all) to make this clearer.  This should probably also be documented as its probably a generally useful pattern.

If this doesn't work as-is, it is my inclination to make this work vs. sticking this in profile_args.  profile args should be arguments to the profile, not just stuff related to profile but that is not passed to its instantiator. IMHO it is cleaner and more maintainable this way vs trying to throw all things as ctor arguments.

The above all assumes that you're calling mozrunner.create per run.  If not, I think the solution is some combination of the above and adding a reset method and/or reworking the .cleanup() logic so that the profile is restored or what not.

I'm happy to help on the above if any of this is unclear.  While there's not a lot of code to be written in any case, it may be....finicky

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Clone profile instead of using it directly. v1.0

Is this what you had in mind, Jeff? It is working well for me.

Comment 4

[Jeff Hammel]

Comment on attachment 733395 [details] [diff] [review]
Clone profile instead of using it directly. v1.0

Yep, right on the money.

Note though that by doing it this way, you are overriding mozrunner.Runner.profile_class globally.  That is, per interpreter.  While this is fine if you only have one place that uses this, as I guess you do here, if you had multiple places that wanted Runner then this could be an issue. IMHO, it is at least clearer to do this at the module-level since, um, once you do it you don't undo it.

But....scratch that.  This can all be prevented by subclassing:

class CloneRunner(Runner):
    profile_class = Profile.clone

self.runner = CloneRunner.create(...)

Comment 5

[Jeff Hammel]

So we should do the things mentioned in comment 2.  Again, it is probably worth re-examining our cleanup/reset logic to make sure things make sense for what we actually care about.  I.e., regardless of this particular bug, it would be nice to have a case where by resetting a clone would cleanup by re-cloning (if that makes sense).  Please leave open until the comment 2 items are done.

:davehunt, does that work for you?  Note that the variable renaming == an API change and whatever you do here (unless you cover both bases, not a horrible idea) will require a change once profile_class -> profile_factory (e.g.).

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm not sure I understand the 'cleanup by re-cloning' comment, but I think everything else makes sense. Did you mean to suggest CloneRunner would become an available subclass in mozrunner, or just in GeckoInstance used by Marionette? I've done the latter, and will attach a patch for review.

I've raised bug 858613 to take care of the profile_class -> profile_factory API change in mozrunner.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Clone profile instead of using it directly. v1.1

Just requesting feedback for now.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Clone profile instead of using it directly. v1.2

Updated after changes from bug 858290 landed.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8a9cfdc055

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for Marionette failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d95f4b59178

https://tbpl.mozilla.org/php/getParsedLog.php?id=21612486&tree=Mozilla-Inbound

15:00:57     INFO - #####
15:00:57     INFO - ##### Running run-marionette step.
15:00:57     INFO - #####
15:00:57     INFO - Running command: ['/builds/slave/talos-slave/test/build/venv/bin/python', '-u', '/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py', '--binary', '/builds/slave/talos-slave/test/build/application/FirefoxNightlyDebug.app/Contents/MacOS/firefox', '--address', 'localhost:2828', '--type', 'browser', '/builds/slave/talos-slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit-tests.ini']
15:00:57     INFO - Copy/paste: /builds/slave/talos-slave/test/build/venv/bin/python -u /builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py --binary /builds/slave/talos-slave/test/build/application/FirefoxNightlyDebug.app/Contents/MacOS/firefox --address localhost:2828 --type browser /builds/slave/talos-slave/test/build/tests/marionette/tests/testing/marionette/client/marionette/tests/unit-tests.ini
15:00:58     INFO -  starting httpd
15:00:58     INFO -  running webserver on http://10.26.56.28:49206/
15:00:58     INFO -  starting runner
15:00:58     INFO -  Traceback (most recent call last):
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 727, in <module>
15:00:58     INFO -      cli()
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 722, in cli
15:00:58     INFO -      runner = startTestRunner(runner_class, options, tests)
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 717, in startTestRunner
15:00:58     INFO -      runner.run_tests(tests, testtype=options.type)
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 361, in run_tests
15:00:58     INFO -      self.run_test(test, testtype)
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 400, in run_test
15:00:58     INFO -      self.start_marionette()
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/runtests.py", line 283, in start_marionette
15:00:58     INFO -      baseurl=self.baseurl)
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/marionette.py", line 208, in __init__
15:00:58     INFO -      self.instance.start()
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/tests/marionette/marionette/geckoinstance.py", line 32, in start
15:00:58     INFO -      cmdargs=['-no-remote'])
15:00:58     INFO -    File "/builds/slave/talos-slave/test/build/venv/lib/python2.7/site-packages/mozrunner/runner.py", line 70, in create
15:00:58     INFO -      profile = cls.profile_class(**(profile_args or {}))
15:00:58     INFO -  TypeError: clone() takes at least 2 arguments (1 given)
15:00:58     INFO -  Exception AttributeError: "'NoneType' object has no attribute 'stop'" in <bound method Marionette.__del__ of <marionette.Marionette object at 0x101637390>> ignored
15:00:58    ERROR - Return code: 1
15:00:58     INFO - TinderboxPrint: marionette: <em class="testfail">T-FAIL</em>
15:00:58    ERROR - Marionette exited with return code 1: harness failures
15:00:58    ERROR - # TBPL FAILURE #

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Clone profile instead of using it directly. v1.3

My mistake was that the CloneRunner was being used regardless of whether a profile was specified. Fixed, tested locally, and pushed to try (thanks Jonathan!)

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

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 735472 [details] [diff] [review]
Clone profile instead of using it directly. v1.3

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

Ah, makes sense, thanks for the fix.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d3e8cc8417

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Uplifted to b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/423f7851bdb5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/1de4724454ec