Closed Bug 879900 Opened 7 years ago Closed 7 years ago

Make Marionette use moztest

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox27 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
mozilla27
Tracking Status
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(3 files, 7 obsolete files)

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.
Ok,

I'll try to focus on it this week :)
Attachment #777195 - Flags: review? → review?(jgriffin)
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-
Yep, oki thx for the review. I'll resubmit a new one soon :)
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)
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)
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-
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-
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)
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)
New submission for moztest part.
Attachment #785866 - Attachment is obsolete: true
Attachment #786915 - Flags: review?(jgriffin)
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?
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 ?
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)
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+
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-
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)
Attachment #787775 - Flags: review?(jgriffin)
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
Attachment #786914 - Attachment is obsolete: true
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
Sounds good, it will take a little work before I can test it anyway.
Depends on: 904682
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: nobody → jgriffin
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+
(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.
Depends on: 907895
Merged to production branch of mozharness.
Live as of now.
Sorry to miss your ping :mbu; I'm still waiting on a few things so we can run this on try.
heh ok, no worries, thanks for your answer :)
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
I think I can run this on try now:  https://tbpl.mozilla.org/?tree=Try&rev=60f8d5676321
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+
Modified a bit to call the TextTestResult constructor.
Attachment #787775 - Attachment is obsolete: true
Comment on attachment 814442 [details] [diff] [review]
Use moztest in Marionette,

Carry r+ forward.
Attachment #814442 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/51320cffafc0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 927404
Blocks: 927606
Blocks: 928842
You need to log in before you can comment on or make changes to this bug.