Closed Bug 886741 Opened 11 years ago Closed 11 years ago

Marionette test runner no longer supports skipped, expected failures, or unexpected successes

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: davehunt, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached file Reduced test case
The fix for bug 874599 has caused a regression in the support for skipping tests, as well as having expected failures or unexpected successes. All of these are now treated as errors.

I have attached a reduced test case for each result type. To replicate:

$ python runtests.py --address=localhost:2828 test_report.py

Expected:
test_error (test_report.TestReport) ... ERROR
test_expected_fail (test_report.TestReport) ... expected failure
test_fail (test_report.TestReport) ... FAIL
test_pass (test_report.TestReport) ... ok
test_skip (test_report.TestReport) ... skipped 'Skip Message'
test_unexpected_pass (test_report.TestReport) ... unexpected success

Actual:
test_error (test_report.TestReport) ... ERROR
test_expected_fail (test_report.TestReport) ... ERROR
test_fail (test_report.TestReport) ... FAIL
test_pass (test_report.TestReport) ... ok
test_skip (test_report.TestReport) ... ERROR
test_unexpected_pass (test_report.TestReport) ... ERROR
Hello Dave,

I'll try to find the time to fix it soon...!

This bug is due to compatibility with python2.6, I did not handled all exception methods in unit test python 2.7. I thought, it wasn't used like this..
Attached patch Add unittest2 exception (obsolete) — Splinter Review
I've added all unittest2 exceptions and add python2.6 compatibility. But i think we have so many unittest codes handled in marionette_test.py.

Plus, the test test_report.py was not python2.6 compliant.
Attachment #767325 - Flags: review?(mdas)
Attachment #767325 - Flags: review?(jgriffin)
Comment on attachment 767325 [details] [diff] [review]
Add unittest2 exception

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

++ for the test!  Can you add the test to unit-tests.ini, so it will get run by default?

::: testing/marionette/client/marionette/marionette_test.py
@@ +91,5 @@
> +        if addSkip is not None:
> +            addSkip(self, reason)
> +        else:
> +            warnings.warn("TestResult has no addSkip method, skips not reported",
> +                          RuntimeWarning, 2)

We should add an addSkip method to MarionetteTestResult in runtests.py, then we can get rid of this 'else' block.

@@ +94,5 @@
> +            warnings.warn("TestResult has no addSkip method, skips not reported",
> +                          RuntimeWarning, 2)
> +            result.addSuccess(self)
> +
> +    def arun(self, result=None):

s/arun/run ?
Attachment #767325 - Flags: review?(jgriffin) → review-
aargg... this a typo.

Ok for the rest.. 

so sorry for the typo :(
Comment on attachment 767325 [details] [diff] [review]
Add unittest2 exception

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

::: testing/marionette/client/marionette/tests/unit/test_report.py
@@ +26,5 @@
> +    def test_unexpected_pass(self):
> +        assert True
> +
> +    def test_error(self):
> +        raise Exception()

This doesn't do what we expect.

If I run this test with python2.7, I get:

FAILED (failures=1, errors=3, skipped=1)

SUMMARY
-------
passed: 1
failed: 4
todo: 1

We get:
ERROR: test_expected_fail (test_report.TestReport)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdas/Code/mozilla-central/testing/marionette/client/marionette/marionette_test.py", line 67, in wrapper
    raise _ExpectedFailure(sys.exc_info())
TEST-UNEXPECTED-FAIL | test_report.py TestReport.test_expected_fail | _ExpectedFailure
======================================================================
ERROR: test_unexpected_pass (test_report.TestReport)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdas/Code/mozilla-central/testing/marionette/client/marionette/marionette_test.py", line 68, in wrapper
    raise _UnexpectedSuccess
TEST-UNEXPECTED-FAIL | test_report.py TestReport.test_unexpected_pass | _UnexpectedSuccess

These two get marked as 'Errors' when they shouldn't be. 

If I run this with python2.6:
FAILED (failures=1, errors=4)

SUMMARY
-------
passed: 1
failed: 5
todo: 0

Where the skipped test gets marked with an error.


What I expect to get is this (if you run the original test_report.py):

FAILED (failures=1, errors=1, skipped=1, expected failures=1, unexpected successes=1)

SUMMARY
-------
passed: 1
failed: 2
todo: 2
Attachment #767325 - Flags: review?(mdas) → review-
hum, it's strange, i get the good output on my laptop.


running webserver on http://192.168.1.2:38911/
TEST-START test_report.py
test_error (test_report.TestReport) ... ERROR
test_expected_fail (test_report.TestReport) ... expected failure
test_fail (test_report.TestReport) ... FAIL
test_pass (test_report.TestReport) ... ok
test_skip (test_report.TestReport) ... skipped 'Skip Message'
test_unexpected_pass (test_report.TestReport) ... unexpected success


FAILED (failures=1, errors=1, skipped=1, expected failures=1, unexpected successes=1)

SUMMARY
-------
passed: 1
failed: 2
todo: 2
Fix typo, add test_report.py in unit-tests.ini.

and addExpectedFailure, addUnexpectedSuccess, addSkip functions in runtests.py
Attachment #767325 - Attachment is obsolete: true
Attachment #767394 - Flags: review?(mdas)
Attachment #767394 - Flags: review?(jgriffin)
Comment on attachment 767394 [details] [diff] [review]
add functions in runtest.py and fix mistake.

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

::: testing/marionette/client/marionette/tests/unit/test_report.py
@@ +26,5 @@
> +    def test_unexpected_pass(self):
> +        assert True
> +
> +    def test_error(self):
> +        raise Exception()

The skip message doesn't get printed. 

Also, you can get weird summary messages if you change some of the tests. For example, if you change line 23 to be True, then the results are unclear:
FAILED (failures=1, errors=1, skipped=1, unexpected successes=2)

SUMMARY
-------
passed: 1
failed: 2
todo: 1

Since there are 6 tests run, but the summary only lists 5.
Attachment #767394 - Flags: review?(mdas) → review-
sorry, the summary only lists 4. I'm not sure what 'todo' is counting now.
Ok,

There were some mistake with unexpectedSuccess in runtest.py. I fix it.
I don't understand why we put skip tests in todo ? We can add a "skip" info on the output.

I give you a patch as soon as possible ...
It fix printing skip comment in the console.
And it fix unexpectedSucess function.
I remove unexpectedFailure from "todo" to "passed".

And I think, we could add a "skip" info instead of "todo" but maybe in a new issue, because this patch contain many changes... :)
Attachment #767394 - Attachment is obsolete: true
Attachment #767394 - Flags: review?(jgriffin)
Attachment #767805 - Flags: review?(mdas)
Attachment #767805 - Flags: review?(jgriffin)
Comment on attachment 767805 [details] [diff] [review]
fix skip message printing, and bad counter on todo and unexpectedSuccesses

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

When I run this locally with 2.7.3, I get:

FAILED (failures=1, errors=1, skipped=1, expected failures=1, unexpected successes=1)

SUMMARY
-------
passed: 2
failed: 3
todo: 1

I would actually expect to see:

passed: 1
failed: 3 (the failure, the error, and the unexpected success)
todo: 2 (the skipped, and the expected error)

Looks very close though!

Once it works correctly locally, we can push it through try...the try run will fail, but we can make sure it fails the same everywhere.  Then, we can remove the test from the manifest (since it's a fail test and we don't have logic in runtests.py to special-case those results) and land it!
Attachment #767805 - Flags: review?(jgriffin) → review-
The reason we don't have a separate skipped category at the end is for TBPL compatibility; TBPL only recognizes passed/failed/todo, so skipped tests traditionally get lumped  into todo, even though it may not be strictly accurate.
Ok,

I have supposed (and took the initiative too ;) ) to put the expectedFailure in the "passed" result instead of "todo" ...
Cause, if your test is to expect a failure, so the is OK-> passed ..? 

But, the revert is very quick ...

I do it as soon as possible :)
An "expected failure" usually indicates some bug in the code or test that needs to be fixed at some point, hence, it's "todo".
revert expectedFailure from "passed" info to "todo"
Attachment #767805 - Attachment is obsolete: true
Attachment #767805 - Flags: review?(mdas)
Attachment #767920 - Flags: review?(mdas)
Attachment #767920 - Flags: review?(jgriffin)
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b38f46c15baa

Theoretically, we should see the same number of pass/fail/todo results for each platform.
I've just tested this locally, and see some issues. We are currently using unittest.expectedFailure in test_debug as seen here: https://github.com/mozilla/gaia-ui-tests/blob/16f8f1c529c02d8c81bf544091e491e98b27aaa2/gaiatest/tests/unit/test_debug.py#L15

With this patch applied, this results in an error being reported instead of an expected failure. I see that there's now an expectedFailure in marionette_test, however I'm unable to import this as it's not exposed in the Marionette client package.

I'm also wondering if I'll be able to do raise a SkipTest from a test's setUp method. I have tested this successfully with unittest.SkipTest and marionette-client 0.5.29, but am uncertain if it's an option with the current patch.
You can currently use expectedFailure with:

from marionette.marionette_test import expectedFailure

However, we should add expectedFailure to Marionette's __init__.py, so you can import it directly from marionette.
SkipTest works with the new patch, but it also needs to be added to __init__.py, in order to avoid having to import it via marionette.marionette_test.
I'm happy to use `from marionette.marionette_test import expectedFailure` if adding these to Marionette's `__init__.py` file is not appropriate.
(In reply to Jonathan Griffin (:jgriffin) from comment #13)
> The reason we don't have a separate skipped category at the end is for TBPL
> compatibility; TBPL only recognizes passed/failed/todo, so skipped tests
> traditionally get lumped  into todo, even though it may not be strictly
> accurate.

aha, thanks.
try run looks great.  I'll update the patch with the __init__.py imports, remove the test from unit-tests.ini, and land it.
Yeah !! Good, i'm happy :)

And, i discovered what TBPL is .. ;)
Attachment #767920 - Flags: review?(mdas) → review+
Removed test from unit-tests.ini, added the imports to __init__.py, and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c2369d8db2
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Target Milestone: --- → mozilla25
Comment on attachment 767920 [details] [diff] [review]
move expectedFailure from passed to todo

And a late-to-the-party r+.
Attachment #767920 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/33c2369d8db2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: