Make Marionette use moztest

RESOLVED FIXED in Firefox 27, Firefox OS v1.2

Status

Testing
Marionette
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgriffin, Assigned: jgriffin)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 fixed, b2g-v1.2 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
Ok,

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

Comment 2

5 years ago
Created attachment 777195 [details] [diff] [review]
First submission, extend TestResultCollection in MarionetteTestResult, update Moztest to be able to use custom subclass of test result
Attachment #777195 - Flags: review?

Updated

5 years ago
Attachment #777195 - Flags: review? → review?(jgriffin)
(Assignee)

Comment 3

5 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

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

Comment 5

5 years ago
Created attachment 785866 [details] [diff] [review]
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
Attachment #777195 - Attachment is obsolete: true
Attachment #785866 - Flags: review?(jgriffin)

Comment 6

5 years ago
Created attachment 785869 [details] [diff] [review]
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
Attachment #785869 - Flags: review?(jgriffin)
(Assignee)

Comment 7

5 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

5 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

5 years ago
Created attachment 786475 [details] [diff] [review]
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.
Attachment #785869 - Attachment is obsolete: true
Attachment #786475 - Flags: review?(jgriffin)

Comment 10

5 years ago
Created attachment 786914 [details] [diff] [review]
bug-879900-fix-marionette-part2.patch

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

5 years ago
Created attachment 786915 [details] [diff] [review]
bug-879900-fix-moztest-part2.patch

New submission for moztest part.
Attachment #785866 - Attachment is obsolete: true
Attachment #786915 - Flags: review?(jgriffin)
(Assignee)

Comment 12

5 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

5 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

5 years ago
Created attachment 787400 [details] [diff] [review]
bug-879900-fix-moztest-part3.patch

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

5 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 17

5 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

5 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

5 years ago
Created attachment 787775 [details] [diff] [review]
bug-879900-fix-marionette-part3.patch
Attachment #787775 - Flags: review?(jgriffin)
(Assignee)

Comment 20

5 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

5 years ago
Attachment #786914 - Attachment is obsolete: true

Comment 21

5 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

5 years ago
Sounds good, it will take a little work before I can test it anyway.
(Assignee)

Updated

5 years ago
Depends on: 904682
(Assignee)

Comment 24

5 years ago
Created attachment 793567 [details] [diff] [review]
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.
Attachment #793567 - Flags: review?(ahalberstadt)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 27

5 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.
(Assignee)

Updated

5 years ago
Depends on: 907895

Comment 28

5 years ago
Merged to production branch of mozharness.
Live as of now.
(Assignee)

Comment 29

5 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

5 years ago
heh ok, no worries, thanks for your answer :)
(Assignee)

Comment 31

5 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

5 years ago
I think I can run this on try now:  https://tbpl.mozilla.org/?tree=Try&rev=60f8d5676321
(Assignee)

Comment 33

4 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

4 years ago
Created attachment 814442 [details] [diff] [review]
Use moztest in Marionette,

Modified a bit to call the TextTestResult constructor.
(Assignee)

Updated

4 years ago
Attachment #787775 - Attachment is obsolete: true
(Assignee)

Comment 35

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

Carry r+ forward.
Attachment #814442 - Flags: review+
(Assignee)

Comment 37

4 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
https://hg.mozilla.org/mozilla-central/rev/51320cffafc0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Blocks: 924997
(Assignee)

Updated

4 years ago
Blocks: 927404
(Assignee)

Updated

4 years ago
Blocks: 927606
(Assignee)

Updated

4 years ago
Blocks: 928842
(Assignee)

Updated

4 years ago
status-b2g-v1.2: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.