Closed Bug 857599 Opened 7 years ago Closed 7 years ago

Clone profile for Marionette tests to prevent polluting the original profile

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 857622
Assuming this is x-platform
OS: Mac OS X → All
Hardware: x86 → All
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
Component: Mozbase → Marionette
QA Contact: hskupin
Summary: Add clone as a profile argument in mozprofile → Clone profile for Marionette tests to prevent polluting the original profile
Is this what you had in mind, Jeff? It is working well for me.
Assignee: nobody → dave.hunt
Attachment #733395 - Flags: feedback?(jhammel)
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(...)
Attachment #733395 - Flags: feedback?(jhammel) → feedback+
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.).
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.
Just requesting feedback for now.
Attachment #733395 - Attachment is obsolete: true
Attachment #733898 - Flags: feedback?(jhammel)
Updated after changes from bug 858290 landed.
Attachment #733898 - Attachment is obsolete: true
Attachment #733898 - Flags: feedback?(jhammel)
Attachment #734369 - Flags: feedback?(jhammel)
Attachment #734369 - Flags: feedback?(jhammel) → feedback+
Attachment #734369 - Flags: review?(jgriffin)
Attachment #734369 - Flags: review?(jgriffin) → review+
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 #
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
Attachment #734369 - Attachment is obsolete: true
Attachment #735472 - Flags: review?(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.
Attachment #735472 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/f9d3e8cc8417
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.