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

RESOLVED FIXED in mozilla32

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Bebe, Assigned: parkouss)

Tracking

({pi-marionette-runner})

unspecified
mozilla32
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [runner])

Attachments

(1 attachment, 3 obsolete attachments)

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
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.
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)
Whiteboard: [runner]
Assignee

Comment 3

5 years ago
I'd like to take the bug. Should I add the behaviour as described in the last comment ?
Flags: needinfo?(jgriffin)
Yes, feel free to implement the suggestion in comment #2.
Flags: needinfo?(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?
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.
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!
Assignee: nobody → j.parkouss
Assignee

Comment 9

5 years ago
Sure - patch corrected as comment #6. Let me know if I misunderstood something.
Attachment #8425736 - Attachment is obsolete: true
Attachment #8425736 - Flags: review?(jgriffin)
Attachment #8426824 - Flags: review?(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.
Attachment #8426824 - Flags: review?(jgriffin) → review-
Assignee

Comment 11

5 years ago
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.
Attachment #8426824 - Attachment is obsolete: true
Attachment #8427568 - Flags: review?(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)
Attachment #8427568 - Flags: review?(jgriffin) → review-
Assignee

Comment 13

5 years ago
Well I misunderstood again; It is now corrected.
Attachment #8427568 - Attachment is obsolete: true
Attachment #8429383 - Flags: review?(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.
Attachment #8429383 - Flags: review?(jgriffin) → review+
Flagging myself to remind me to push it to try.
Flags: needinfo?(jgriffin)
try is happy!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/99d60f78b543
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.