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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: 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

Comment 1

[mathieu bultel:mbu]

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

Comment 2

[mathieu bultel:mbu]

Attachment: Add unittest2 exception

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.

Comment 3

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

Comment 4

[mathieu bultel:mbu]

aargg... this a typo.

Ok for the rest.. 

so sorry for the typo :(

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 6

[mathieu bultel:mbu]

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

Comment 7

[mathieu bultel:mbu]

Attachment: add functions in runtest.py and fix mistake.

Fix typo, add test_report.py in unit-tests.ini.

and addExpectedFailure, addUnexpectedSuccess, addSkip functions in runtests.py

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

sorry, the summary only lists 4. I'm not sure what 'todo' is counting now.

Comment 10

[mathieu bultel:mbu]

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

Comment 11

[mathieu bultel:mbu]

Attachment: fix skip message printing, and bad counter on todo and unexpectedSuccesses

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

Comment 12

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

Comment 13

[Jonathan Griffin (:jgriffin)]

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.

Comment 14

[mathieu bultel:mbu]

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

Comment 15

[Jonathan Griffin (:jgriffin)]

An "expected failure" usually indicates some bug in the code or test that needs to be fixed at some point, hence, it's "todo".

Comment 16

[mathieu bultel:mbu]

Attachment: move expectedFailure from passed to todo

revert expectedFailure from "passed" info to "todo"

Comment 17

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

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 19

[Jonathan Griffin (:jgriffin)]

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.

Comment 20

[Jonathan Griffin (:jgriffin)]

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.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm happy to use `from marionette.marionette_test import expectedFailure` if adding these to Marionette's `__init__.py` file is not appropriate.

Comment 22

[Malini Das [:mdas] - Away, not checking bugmail]

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

Comment 23

[Jonathan Griffin (:jgriffin)]

try run looks great.  I'll update the patch with the __init__.py imports, remove the test from unit-tests.ini, and land it.

Comment 24

[mathieu bultel:mbu]

Yeah !! Good, i'm happy :)

And, i discovered what TBPL is .. ;)

Comment 25

[Jonathan Griffin (:jgriffin)]

Removed test from unit-tests.ini, added the imports to __init__.py, and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c2369d8db2

Comment 26

[Jonathan Griffin (:jgriffin)]

Comment on attachment 767920 [details] [diff] [review]
move expectedFailure from passed to todo

And a late-to-the-party r+.

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/33c2369d8db2

Comment 28

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/728837609e47

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/728837609e47