879900 - Make Marionette use moztest

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

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

[mathieu bultel:mbu]

Ok,

I'll try to focus on it this week :)

Comment 2

[mathieu bultel:mbu]

Attachment: First submission, extend TestResultCollection in MarionetteTestResult, update Moztest to be able to use custom subclass of test result

Comment 3

[Jonathan Griffin (: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

Comment 4

[mathieu bultel:mbu]

Yep, oki thx for the review. I'll resubmit a new one soon :)

Comment 5

[mathieu bultel:mbu]

Attachment: bug-879900-fix-moztest-part.patch

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

Comment 6

[mathieu bultel:mbu]

Attachment: bug-879900-fix-marionette-part.patch

Simply add the properties for skipped, expectedFailures, unexpectedSuccesses and tests_passed into MarionetteTestResult to store the result into the Moztest result collection

Comment 7

[Jonathan Griffin (: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.

Comment 8

[Jonathan Griffin (:jgriffin)]

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).

Comment 9

[mathieu bultel:mbu]

Attachment: bug-879900-fix-marionette-part-2.patch

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.

Comment 10

[mathieu bultel:mbu]

Attachment: bug-879900-fix-marionette-part2.patch

Change a little marionette part for moztest.add_result refactoring

Comment 11

[mathieu bultel:mbu]

Attachment: bug-879900-fix-moztest-part2.patch

New submission for moztest part.

Comment 12

[Jonathan Griffin (: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?

Comment 13

[mathieu bultel:mbu]

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

[mathieu bultel:mbu]

Attachment: bug-879900-fix-moztest-part3.patch

add get_class into add_result function
Change test_name property.

Comment 15

[Jonathan Griffin (: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.

Comment 16

[Jonathan Griffin (:jgriffin)]

https://github.com/mozilla/mozbase/commit/08dcb5d1bfd4bd4c6cb9cc1ab56dcacd7d5b052b

Comment 17

[Jonathan Griffin (:jgriffin)]

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.

Comment 18

[mathieu bultel:mbu]

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

[mathieu bultel:mbu]

Attachment: bug-879900-fix-marionette-part3.patch

Comment 20

[Jonathan Griffin (: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

Comment 21

[mathieu bultel:mbu]

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

Comment 22

[Jonathan Griffin (:jgriffin)]

Sounds good, it will take a little work before I can test it anyway.

Comment 23

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5d0624a4b987

Comment 24

[Jonathan Griffin (:jgriffin)]

Attachment: Install moztest in mozharness marionette script,

We want to start using moztest in Marionette tests; also fixed some weird non-PEP spacing in this code block.

Comment 25

[Andrew Halberstadt [:ahal]]

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.

Comment 26

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/build/mozharness/rev/5d65ed9fbd0f

Comment 27

[Jonathan Griffin (:jgriffin)]

(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

[Armen [:armenzg]]

Merged to production branch of mozharness.
Live as of now.

Comment 29

[Jonathan Griffin (:jgriffin)]

Sorry to miss your ping :mbu; I'm still waiting on a few things so we can run this on try.

Comment 30

[mathieu bultel:mbu]

heh ok, no worries, thanks for your answer :)

Comment 31

[Jonathan Griffin (:jgriffin)]

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.

Comment 32

[Jonathan Griffin (:jgriffin)]

I think I can run this on try now:  https://tbpl.mozilla.org/?tree=Try&rev=60f8d5676321

Comment 33

[Jonathan Griffin (:jgriffin)]

Comment on attachment 787775 [details] [diff] [review]
bug-879900-fix-marionette-part3.patch

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

All green on try!

Comment 34

[Jonathan Griffin (:jgriffin)]

Attachment: Use moztest in Marionette,

Modified a bit to call the TextTestResult constructor.

Comment 35

[Jonathan Griffin (:jgriffin)]

Comment on attachment 814442 [details] [diff] [review]
Use moztest in Marionette,

Carry r+ forward.

Comment 36

[Jonathan Griffin (:jgriffin)]

new try run: https://tbpl.mozilla.org/?tree=Try&rev=f9d263a91b02

Comment 37

[Jonathan Griffin (:jgriffin)]

and finally, after way too much time:  https://hg.mozilla.org/integration/mozilla-inbound/rev/51320cffafc0

Thanks mbu for all the patience!

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51320cffafc0

Comment 39

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3141fa09989e