656829 - xpcshell harness treats a todo unexpected pass as "the whole test passed" instead of as a failure

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Daniel Holbert [:dholbert]]

"todo" is busted in xpcshell tests.

If a "todo" check unexpectedly passes, it aborts the whole test (which I think is good, because that's what "real" failures do too) -- BUT -- it's then treated as if the whole test *passed* rather than failed.

Sample XPCshell test:

> function run_test()
> {
>  todo_check_true(true); // This unexpected pass should make the test be considered a failure
>  do_check_true(false);  // This failure should make the test be considered a failure
> }

If I stick that in a directory of xpcshell tests and run xpcshell tests on that directory, it behaves as if the test passed.

If I run "make ... check-one" to get more verbose & targeted test output, I get
a line about TEST-UNEXPECTED-PASS some debug information printed, but the output still ends with
> INFO | Result summary:
> INFO | Passed: 1
> INFO | Failed: 0

Mossop says on IRC:
> Yeah, the test output parser doesn't understand TEST-UNEXPECTED-PASS :(
> http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#508

Comment 1

[Daniel Holbert [:dholbert]]

Taking a crack at this, looks like it should be straightforward...

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This strikes me as the most straightforward fix -- basically treat any "TEST-UNEXPECTED-*" on any subtest as a "TEST-UNEXPECTED-FAIL" for the test as a whole.

(Alternately, we could specifically look for TEST-UNEXPECTED-PASS and then print out "TEST-UNEXPECTED-PASS" for the whole test, but I don't think that makes as much sense.  IIUC, the message that's printed out inside this "if" clause describes the test *as a whole*, and here, the test _as a whole_ didn't unexpectedly pass. In fact, we probably didn't even run all of the checks in the whole test -- we'll have aborted early -- and generally, we *do* expect the test (as a whole) to pass.  Plus, we want to increment the failure count, not the pass count, for this situation.  So I think that labeling it a "FAIL" for the test as a whole makes sense.)

Comment 3

[(not currently active) Ted Mielczarek]

Ick. Can't believe I missed that.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

doh

Comment 5

[Daniel Holbert [:dholbert]]

I ran this on TryServer last night to see what oranges have been masked by this bug, and mercifully, there was only 1 test with issues: test_hunspell.js.  Filed bug 656944 on fixing that test up.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 532110 [details] [diff] [review]
fix v1

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

Looks good. Thanks for finding and fixing this!

Comment 7

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/projects/cedar/rev/c4393d83e34d

Comment 8

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/c4393d83e34d