648127 - A test failure right after something is dumped without a trailing \n goes unnoticed

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: WIP

STR:
Create an xpcshell test with the following code:

function run_test() {
  dump("foo");
  do_check_true(false);
}

The test will pass even though it should fail.

Attached is a WIP patch that prints newlines before each TEST-UNEXPECTED-FAIL, per ted's recommendation. The patch is currently running through the tryserver.

Jason, you wrote the code that prints parent/child for xpcshell tests. Do you know why some but not all the TEST-UNEXPECTED-FAIL statements use _dump()?

Comment 1

[Rain [:rain1] (inactive)]

Attachment: patch, v1

Comment 2

[Rain [:rain1] (inactive)]

Attachment: Patch to check in

Comment 3

[Rain [:rain1] (inactive)]

I'm a bit short on time to tend to the tree right now, so it'd be great if someone else checked this in.

Comment 4

[Jason Duell]

Comment on attachment 526715 [details] [diff] [review]
Patch to check in

The "newline" parameter strikes me as gross.

What we're trying to do here is make sure any TEST-UNEXPECTED-FAIL shows up at the start of a line.  Seems cleaner to just add a "test_unexpected_fail()" function that prepends "\nTEST-UNEXPECTED-FAIL | " (or "\nTEST-UNEXPECTED-FAIL | (xpcshell/head.js) | ": we're not consistent in our usage) to whatever string it gets.

Comment 5

[Jason Duell]

> why some but not all the TEST-UNEXPECTED-FAIL statements use _dump()?

They should all use _dump, as should any "top-level" logging statement (i.e. printing "TEST-INFO" should use it on line 488, but is currently using dump: let's fix that), but if you're just printing supplemental info for a previous _dump (like dumping out stack info) you can use dump if you don't want each line to state if it comes from the parent or child.

Comment 6

[Rain [:rain1] (inactive)]

(In reply to comment #4)
> Comment on attachment 526715 [details] [diff] [review]
> Patch to check in
> 
> The "newline" parameter strikes me as gross.
> 
> What we're trying to do here is make sure any TEST-UNEXPECTED-FAIL shows up at
> the start of a line.  Seems cleaner to just add a "test_unexpected_fail()"
> function that prepends "\nTEST-UNEXPECTED-FAIL | " (or "\nTEST-UNEXPECTED-FAIL
> | (xpcshell/head.js) | ": we're not consistent in our usage) to whatever string
> it gets.

Yeah the newline parameter was a hack. I'll put up an updated patch when I have the chance.

Comment 7

[Jason Duell]

Maybe an even simpler idea:  just have _dump check for 'TEST-' at start of message, and print \n first if it's a match.   That would ensure that all the TEST- results start on a newline, not just TEST-UNEXPECTED-FAILURE.

Comment 8

[Rain [:rain1] (inactive)]

Attachment: patch v2

Yeah, that sounds best.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 528205 [details] [diff] [review]
patch v2

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

This is fine too.

Comment 10

[Rain [:rain1] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8f79761f3ea6