1225781 - ./mach xpcshell-test no longer exits with code 1 if a test fails

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Mark Banner (:standard8)]

I just noticed that forcing a xpcshell-test to fail and doing something like:

./mach xpcshell-test browser/components/loop/test/xpcshell/test_loopservice_initialize.js; echo $?

shows '0' rather than '1'.

We were using this in one of our helper scripts on Hello, to ensure that the script stopped when tests failed.

I tracked the regression range to bug 1193257.

Comment 1

[Mark Banner (:standard8)]

So something did change, but the fix may be more complicated. On re-reading the code before bug 1193257 landed, it looks like:

- `./mach xpcshell` would always return 0, even if there was failure (presumably due to making the test servers happy)
- `./mach xpcshell <test path>` would return 1 on failure, 0 on pass.

This probably needs someone slightly more familiar with the code to look at it ;-)

Comment 2

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1225781: Return exit status of tests,

Review commit: https://reviewboard.mozilla.org/r/83730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83730/

Comment 3

[Ethan Glasser-Camp (:glasserc)]

I also discovered this behavior by accident. I'm not 100% sure I understand :standard8's comment #1 (why would the test servers need the xpcshell command to return 0 if there are failures?) but it was easy enough to add code that only returns the exit status if paths were passed to it. I tagged :ahal as a reviewer because he reviewed the bug that introduced the regression, feel free to change that if it's wrong!

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8798209 [details]
Bug 1225781: Return exit status of tests,

https://reviewboard.mozilla.org/r/83730/#review82454

::: testing/xpcshell/mach_commands.py:390
(Diff revision 1)
> -            return xpcshell.run_test(**params)
> +            status = xpcshell.run_test(**params)
>          except InvalidTestPathError as e:
>              print(e.message)
>              return 1
> +
> +        if params.get("testPaths"):
> +            return status

We should always return the status. I think the only problem was that run_test wasn't returning self.run_suite, which means it returns None which python then converts to 0.

So remove this change and I think the patch would be good. Though would still be a good idea to verify the original problem is fixed.

Comment 5

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8798209 [details]
Bug 1225781: Return exit status of tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83730/diff/1-2/

Comment 6

[Ethan Glasser-Camp (:glasserc)]

Comment on attachment 8798209 [details]
Bug 1225781: Return exit status of tests,

https://reviewboard.mozilla.org/r/83730/#review82454

> We should always return the status. I think the only problem was that run_test wasn't returning self.run_suite, which means it returns None which python then converts to 0.
> 
> So remove this change and I think the patch would be good. Though would still be a good idea to verify the original problem is fixed.

Is there a test suite or something that I could add a unit test to? I verified that the change worked by adding a failure to an xpcshell test using `equal(1, 2)` and then running it with `./mach xpcshell-test path/to/failing/test.js; echo $?`.

Comment 7

[(not currently active) Ted Mielczarek]

We have tests for the xpcshell harness, but not for any of the mach commands, sadly.

Comment 8

[Andrew Halberstadt [:ahal]]

And there is actually a test for this in selftest.py already:
https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/selftest.py#837

So at least we can be confident it's working in automation I guess :/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f0f7bba7277
Return exit status of tests, r=ahal

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f0f7bba7277