Closed Bug 656829 Opened 11 years ago Closed 11 years ago

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


(Testing :: XPCShell Harness, defect)

Not set


(Not tracked)



(Reporter: dholbert, Assigned: dholbert)




(1 file)

"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 :(
Taking a crack at this, looks like it should be straightforward...
Assignee: nobody → dholbert
Summary: xpcshell harness treats a todo failure as "the whole test passed" → xpcshell harness treats a todo unexpected pass as "the whole test passed" instead of as a failure
OS: Linux → All
Hardware: x86 → All
Attached patch fix v1Splinter Review
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.)
Attachment #532110 - Flags: review?(ted.mielczarek)
Ick. Can't believe I missed that.
Blocks: 629734
Depends on: 656944
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 on attachment 532110 [details] [diff] [review]
fix v1

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

Looks good. Thanks for finding and fixing this!
Attachment #532110 - Flags: review?(ted.mielczarek) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.