940924 - Marionette should list the number of unexpected/expected fails at the end of the test run

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Florin Strugariu [:Bebe]]

now at the end of a test run we see:
passed: 66
failed: 1
todo: 69

FAILED TESTS
-------
test_lockscreen_unlock_to_homescreen.py test_lockscreen_unlock_to_homescreen.TestLockScreen.test_unlock_to_homescreen


But the failed tests is actually a unexpected success.

Can we have a report that makes a difference on what's what:

passed: 66
failed: 0
unexpected: 1
todo: 69

UNEXPECTED SUCCESS TESTS
-------
test_lockscreen_unlock_to_homescreen.py test_lockscreen_unlock_to_homescreen.TestLockScreen.test_unlock_to_homescreen

Comment 1

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

An unexpected success is considered a failure. These are called out in the HTML report though, does that meet your needs? I suspect that changing this console output may have an affect on how TBPL identifies failures, but jgriffin would probably know that better than me.

Similarly, we don't currently call out the expected failures, and skipped tests are identified as 'todo'. I expect that with TBPL 2 we won't need to be so restricted.

Comment 2

[Jonathan Griffin (:jgriffin)]

For TBPL's benefit, we need the list of passed/failed/todo counts as they are today; we could add additional information about them though, e.g.,

passed: 10
failed: 5 (unexpected successes: 3)
todo: 2 (skipped: 2)

Comment 3

[Julien Pagès (:parkouss)]

I'd like to take the bug. Should I add the behaviour as described in the last comment ?

Comment 4

[Jonathan Griffin (:jgriffin)]

Yes, feel free to implement the suggestion in comment #2.

Comment 5

[Julien Pagès (:parkouss)]

Attachment: Marionette should list the number of unexpected/expected fails at the end of the test run

Comment 6

[Jonathan Griffin (:jgriffin)]

Looking at this patch and related code, I can see we're treating known failures a bit different than we do for other harnesses, like mochitest.  In mochitest, a known failure is marked as a 'todo'; in Marionette, we add it to 'passed'.

If we keep the current sense of 'todo', then there isn't any need to break out skipped tests, since todo always means skipped.  Alternately, and more accurately, we could make known failures increment todo, in which case breaking out the skipped tests makes sense.

Zac or Dave, do you have an opinion on this?

Comment 7

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

Not a strong opinion. I can see that it's misleading to count a known failure as a passed test. The most aesthetically pleasing solution to me would be to list passed tests, expected failures, (unexpected) failures, and skipped tests, but we're restricted by TBPL, and I don't object to having 'todo' to bundle both expected failures and skipped tests.

Comment 8

[Jonathan Griffin (:jgriffin)]

j.parkouss, can you amend this patch per comment #6?  I.e., move expectedFailures from passed to todo, and NOT count those in skipped?  Thanks!

Comment 9

[Julien Pagès (:parkouss)]

Attachment: Marionette should list the number of unexpected/expected fails at the end of the test run

Sure - patch corrected as comment #6. Let me know if I misunderstood something.

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8426824 [details] [diff] [review]
Marionette should list the number of unexpected/expected fails at the end of the test run

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

::: testing/marionette/client/marionette/runner/base.py
@@ +812,1 @@
>          self.logger.info('todo: %d' % self.todo)

Here, we can now write something like:

'todo: 5 (skipped: 3)', assuming that 2 of the 5 failures were expectedFailures, and the rest were skipped.

Comment 11

[Julien Pagès (:parkouss)]

Attachment: Marionette should list the number of unexpected/expected fails at the end of the test run

Sorry for the inconvenience;

With the new patch, i can see an output like this:

> OK (skipped=1, expected failures=2, unexpected successes=1)
> 
> SUMMARY
> -------
> passed: 16
> failed: 1 (unexpected sucesses: 1)
> todo: 2 (skipped: 1)

when I inject errors like this:

@skip("skip")
def testSkip(self):
    pass
    
@expectedFailure
def testIt(self):
    assert True

@expectedFailure
def testIt1(self):
    assert False

@expectedFailure
def testIt2(self):
    assert False

Nothe that I have to import skip and expectedFailure from marionette_test (and not from unittest) for this to work; but I thinks it's a known behaviour.

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8427568 [details] [diff] [review]
Marionette should list the number of unexpected/expected fails at the end of the test run

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

::: testing/marionette/client/marionette/runner/base.py
@@ +979,5 @@
>              self.results.append(results)
>  
>              self.failed += len(results.failures) + len(results.errors)
>              if hasattr(results, 'skipped'):
> +                self.skipped += len(results.skipped)

I think we want to add len(results.skipped) to both self.skipped and self.todo, similar to how we add len(results.unexpectedSuccesses) to self.failed and self.unexpected_successes.  This way, our logging will look like:

todo: 5 (skipped: 2)  (assuming 5 total tests, 2 of which were skipped)

rather than

todo: 3 (skipped: 2)

Comment 13

[Julien Pagès (:parkouss)]

Attachment: Marionette should list the number of unexpected/expected fails at the end of the test run

Well I misunderstood again; It is now corrected.

Comment 14

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8429383 [details] [diff] [review]
Marionette should list the number of unexpected/expected fails at the end of the test run

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

lgtm!  I'll run this on try.

Comment 15

[Jonathan Griffin (:jgriffin)]

Flagging myself to remind me to push it to try.

Comment 16

[Jonathan Griffin (:jgriffin)]

https://tbpl.mozilla.org/?tree=Try&rev=79df5e2d3920

Comment 17

[Jonathan Griffin (:jgriffin)]

try is happy!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99d60f78b543

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99d60f78b543