Closed
Bug 879900
Opened 10 years ago
Closed 10 years ago
Make Marionette use moztest
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox27 fixed, b2g-v1.2 fixed)
RESOLVED
FIXED
mozilla27
People
(Reporter: jgriffin, Assigned: jgriffin)
References
Details
Attachments
(3 files, 7 obsolete files)
7.96 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
7.60 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
It would be nice if Marionette used moztest to keep track of test results. Currently, we use a subclass of unittest._TextTestResult (http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runtests.py#27); the disadvantage of this is that it's hard to add custom test properties (like a screenshot). I think it would be useful to make MarionetteTestResult extend moztest's TestResultCollection (as well as unittest._TextTestResult) and make errors[], failures[], and tests_passed[] properties that operate on the collection's members. In order to support the use case of being able to add custom test properties, we'd also need to modify moztest to do this; currently TestResultCollection is hardcoded to use TestResult directly; there's no provision for use of a subclass that could define additional instance variables.
Comment 1•10 years ago
|
||
Ok, I'll try to focus on it this week :)
Comment 2•10 years ago
|
||
Attachment #777195 -
Flags: review?
Updated•10 years ago
|
Attachment #777195 -
Flags: review? → review?(jgriffin)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 777195 [details] [diff] [review] First submission, extend TestResultCollection in MarionetteTestResult, update Moztest to be able to use custom subclass of test result Review of attachment 777195 [details] [diff] [review]: ----------------------------------------------------------------- Good first iteration, and kudos for the test case! Please add moztest to Marionette's setup.py. For mozbase patches, we handle them a little differently than a typical gecko patch, so can you break the mozbase parts into a separate patch? Thanks! ::: testing/marionette/client/marionette/runtests.py @@ +37,2 @@ > self.passed = 0 > self.skipped = [] Instead of maintaining separate lists of skipped, expectedFailures, etc, we can now make these properties which would do something like: @property def skipped(self): return this.filter(lambda t: t.result == t.SKIPPED) See the tests_with_result method of TestResultCollection. Things which do e.g., len(skipped) could do count(skipped) (from moztest.output.base) instead. Or, we could have the skipped property return an actual list instead of a generator. @@ +188,5 @@ > if not result.wasSuccessful(): > self.stream.write("FAILED") > failed, errored = map(len, (result.failures, result.errors)) > if failed: > + result.append("failures=%d" % failed) We don't want to append any strings to the result list; it should only contain instances of TestResult. All these strings are just for dumping to the output, so we can just leave the original implementation intact. ::: testing/mozbase/moztest/moztest/results.py @@ +189,5 @@ > > class TestResultCollection(list): > """ Container class that stores test results """ > > + results = TestResult I think this variable should be named resultClass, for clarity. Then, in __init__, we can just do: if resultClass is not None: self.resultClass = resultClass
Attachment #777195 -
Flags: review?(jgriffin) → review-
Comment 4•10 years ago
|
||
Yep, oki thx for the review. I'll resubmit a new one soon :)
Comment 5•10 years ago
|
||
After, many many days :) this is the second submission. This is Moztest part. I add two function. One property in TestResult, named "test_name", to get a complete name of a test result. Second, to add test result in the collection. I think, add_result function could be refactor and being used by add_unittest_result function
Attachment #777195 -
Attachment is obsolete: true
Attachment #785866 -
Flags: review?(jgriffin)
Comment 6•10 years ago
|
||
Simply add the properties for skipped, expectedFailures, unexpectedSuccesses and tests_passed into MarionetteTestResult to store the result into the Moztest result collection
Attachment #785869 -
Flags: review?(jgriffin)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 785866 [details] [diff] [review] bug-879900-fix-moztest-part.patch Review of attachment 785866 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I agree we should change add_test_result to use add_result; can you make this change? Then, I think it will be good to land.
Attachment #785866 -
Flags: review?(jgriffin) → review-
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 785869 [details] [diff] [review] bug-879900-fix-marionette-part.patch Review of attachment 785869 [details] [diff] [review]: ----------------------------------------------------------------- This is looking much better. In order to be complete, I think we'll need to add overrides for addError and addFailure which populate the TestResultCollection appropriately. Also, I think we can dispense with the property setters, since they are only used in addSuccess/addUnexpectedFailure/addFoo, and just call add_result from within those methods. ::: testing/marionette/client/marionette/runtests.py @@ +73,3 @@ > > def addSuccess(self, test): > super(MarionetteTestResult, self).addSuccess(test) Since we're going to track the successful tests using TestResultCollection, we should omit the call to super() here (even though the base method is a no-op, at least in python 2.7.3).
Attachment #785869 -
Flags: review?(jgriffin) → review-
Comment 9•10 years ago
|
||
Ok, I've added errors and failures properties to the collection. I have made some little changes in printErrorList function. For unit testing this part i use the very usefull "test_report.py" test, written by Dave Hunt (https://bug886741.bugzilla.mozilla.org/attachment.cgi?id=767099). I didn't add it to the patch .. but i can add it as an attachment just for test. I will landed, the next patch for moztest asap.
Attachment #785869 -
Attachment is obsolete: true
Attachment #786475 -
Flags: review?(jgriffin)
Comment 10•10 years ago
|
||
Change a little marionette part for moztest.add_result refactoring
Attachment #786475 -
Attachment is obsolete: true
Attachment #786475 -
Flags: review?(jgriffin)
Attachment #786914 -
Flags: review?(jgriffin)
Comment 11•10 years ago
|
||
New submission for moztest part.
Attachment #785866 -
Attachment is obsolete: true
Attachment #786915 -
Flags: review?(jgriffin)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 786915 [details] [diff] [review] bug-879900-fix-moztest-part2.patch Review of attachment 786915 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozbase/moztest/moztest/results.py @@ +245,5 @@ > + result_actual='PASS', output='', context=None): > + def get_class(test): > + return test.__class__.__module__ + '.' + test.__class__.__name__ > + > + t = self.resultClass(name=test._testMethodName, test_class=test.__class__, Shouldn't we use get_class here, instead of test.__class__, to preserve the original behavior? Does str(test).split()[0] (from the original) evaluate to the same as test._testMethodName?
Comment 13•10 years ago
|
||
Yes, it's a mistake from my part. I add get_class especially for preserving the original behavior. I'll update this soon. For str(test).split()[0] VS test._testMethodName, this evaluate the same. But I suppose it's better to use the original (str(test).split()[0]) because _testMethodName should be a private function ?
Comment 14•10 years ago
|
||
add get_class into add_result function Change test_name property.
Attachment #786915 -
Attachment is obsolete: true
Attachment #786915 -
Flags: review?(jgriffin)
Attachment #787400 -
Flags: review?(jgriffin)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 787400 [details] [diff] [review] bug-879900-fix-moztest-part3.patch Review of attachment 787400 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks, I'll land this in github.
Attachment #787400 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://github.com/mozilla/mozbase/commit/08dcb5d1bfd4bd4c6cb9cc1ab56dcacd7d5b052b
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 786914 [details] [diff] [review] bug-879900-fix-marionette-part2.patch Review of attachment 786914 [details] [diff] [review]: ----------------------------------------------------------------- Cool, almost there! ::: testing/marionette/client/marionette/runtests.py @@ +41,5 @@ > + > + @skipped.setter > + def skipped(self, test): > + if test != []: > + self.add_result(test[0], output=test[1], result_actual='SKIPPED') let's just delete the setters, like this one, and... @@ +134,5 @@ > self.stream.write("u") > self.stream.flush() > > def addSkip(self, test, reason): > + self.skipped = ((test, reason)) ...call add_result() directly from addSkip, addUnexpectedSuccess, etc, since the pattern: self.skipped = foo is quite misleading wrt to what's actually happening here.
Attachment #786914 -
Flags: review?(jgriffin) → review-
Comment 18•10 years ago
|
||
Ok, but just a little thing : if we remove the setter, unittest.TestResult will initialize skipped list and other lists in the __init__ part : self.failures = [] self.errors = [] self.testsRun = 0 self.skipped = [] self.expectedFailures = [] self.unexpectedSuccesses = [] So, we can remove the call of unittest.TestResult.__init__ and set specific attributes in marionette's __init__ function (see new attachment for more details)
Comment 19•10 years ago
|
||
Attachment #787775 -
Flags: review?(jgriffin)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 787775 [details] [diff] [review] bug-879900-fix-marionette-part3.patch Review of attachment 787775 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, I think this may work, I'd like to run it on try, but I'll have to wait until I merge those moztest changes to m-c. ::: testing/marionette/client/marionette/runtests.py @@ +64,5 @@ > + > + @property > + def errors(self): > + return [t for t in self if t.result == 'ERROR'] > + nit: extra whitespace here
Assignee | ||
Updated•10 years ago
|
Attachment #786914 -
Attachment is obsolete: true
Comment 21•10 years ago
|
||
Oki, no worries, i'm on holidays, i'm not sure to still have a network connection for the next days. If you're ok with the last attachment, when you'll run it, i could submit a last patch without the extra whitespaces on line 68
Assignee | ||
Comment 22•10 years ago
|
||
Sounds good, it will take a little work before I can test it anyway.
Assignee | ||
Comment 23•10 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5d0624a4b987
Assignee | ||
Comment 24•10 years ago
|
||
We want to start using moztest in Marionette tests; also fixed some weird non-PEP spacing in this code block.
Attachment #793567 -
Flags: review?(ahalberstadt)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jgriffin
Comment 25•10 years ago
|
||
Comment on attachment 793567 [details] [diff] [review] Install moztest in mozharness marionette script, Review of attachment 793567 [details] [diff] [review]: ----------------------------------------------------------------- Actually just today chmanchester was needing moztest included in the b2g/desktop unittests as well. I guess we can file a separate bug for that.
Attachment #793567 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/build/mozharness/rev/5d65ed9fbd0f
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #26) > https://hg.mozilla.org/build/mozharness/rev/5d65ed9fbd0f backed out for busting everything: http://hg.mozilla.org/build/mozharness/rev/c3d824b09c1f 12:27:48 ERROR - Traceback (most recent call last): 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/pip-0.8.2-py2.6.egg/pip/basecommand.py", line 130, in main 12:27:48 INFO - self.run(options, args) 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/pip-0.8.2-py2.6.egg/pip/commands/install.py", line 195, in run 12:27:48 INFO - InstallRequirement.from_line(name, None)) 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/pip-0.8.2-py2.6.egg/pip/req.py", line 101, in from_line 12:27:48 INFO - return cls(req, comes_from, url=url) 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/pip-0.8.2-py2.6.egg/pip/req.py", line 41, in __init__ 12:27:48 INFO - req = pkg_resources.Requirement.parse(req) 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/distribute-0.6.14-py2.6.egg/pkg_resources.py", line 2547, in parse 12:27:48 INFO - reqs = list(parse_requirements(s)) 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/distribute-0.6.14-py2.6.egg/pkg_resources.py", line 2472, in parse_requirements 12:27:48 INFO - line, p, specs = scan_list(VERSION,LINE_END,line,p,(1,2),"version spec") 12:27:48 INFO - File "/home/cltbld/talos-slave/test/build/venv/lib/python2.6/site-packages/distribute-0.6.14-py2.6.egg/pkg_resources.py", line 2440, in scan_list 12:27:48 INFO - raise ValueError("Expected "+item_name+" in",line,"at",line[p:]) 12:27:48 INFO - ValueError: ('Expected version spec in', 'tests/mozbase/moztest', 'at', '/mozbase/moztest') 12:27:48 INFO - Storing complete log in /home/cltbld/.pip/pip.log 12:27:48 ERROR - Return code: 2 Apparently, we also need to add moztest to tests.zip.
Comment 28•10 years ago
|
||
Merged to production branch of mozharness. Live as of now.
Assignee | ||
Comment 29•10 years ago
|
||
Sorry to miss your ping :mbu; I'm still waiting on a few things so we can run this on try.
Comment 30•10 years ago
|
||
heh ok, no worries, thanks for your answer :)
Assignee | ||
Comment 31•10 years ago
|
||
Turns out I can't install moztest in tests.zip for the mozilla-release branch easily, so I think this needs to depend on bug 908356.
Depends on: 908356
Assignee | ||
Comment 32•10 years ago
|
||
I think I can run this on try now: https://tbpl.mozilla.org/?tree=Try&rev=60f8d5676321
Assignee | ||
Comment 33•10 years ago
|
||
Comment on attachment 787775 [details] [diff] [review] bug-879900-fix-marionette-part3.patch Review of attachment 787775 [details] [diff] [review]: ----------------------------------------------------------------- All green on try!
Attachment #787775 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 34•10 years ago
|
||
Modified a bit to call the TextTestResult constructor.
Assignee | ||
Updated•10 years ago
|
Attachment #787775 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Comment on attachment 814442 [details] [diff] [review] Use moztest in Marionette, Carry r+ forward.
Attachment #814442 -
Flags: review+
Assignee | ||
Comment 36•10 years ago
|
||
new try run: https://tbpl.mozilla.org/?tree=Try&rev=f9d263a91b02
Assignee | ||
Comment 37•10 years ago
|
||
and finally, after way too much time: https://hg.mozilla.org/integration/mozilla-inbound/rev/51320cffafc0 Thanks mbu for all the patience!
Target Milestone: --- → mozilla27
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/51320cffafc0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
status-b2g-v1.2:
--- → fixed
status-firefox27:
--- → fixed
Assignee | ||
Comment 39•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3141fa09989e
Updated•4 months ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•