704708 - failing test doesn't flunk 'cfx test'

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Brian Warner [:warner :bwarner]]

While doing some work (and breaking things), I noticed that unfortunately 'cfx testall' still reports "All tests were successful. Ship it!" even when one or more of the JS unit tests have failed.

The easiest way to reproduce this is to edit any test in packages/api-utils/tests/ to signal an error, then run 'cfx test' from package/api-utils. In my run, it finished with "2637 of 2638 tests passed.", "Total time:..", and "Program terminated successfully". The code in "cfx testall" relies upon the individual package's "cfx test" to exit *un*successfully (retval != 0) to indicate an error.

I think something must have gotten dropped when we changed the way 'cfx run' works. It looks like "OK" is being written to the resultsfile even though a test had failed, so I'm suspecting something in the unit-test running code (like unit-test.js or unit-test-finder.js).

This is unfortunate, because without "cfx testall" printing a failure message, it's hard to spot the difference between tests-passed and tests-run, to realize that there's a problem. Worse, the buildbot usually depends on the exit status of "cfx testall" to know whether to declare a problem or not.

Comment 1

[Brian Warner [:warner :bwarner]]

I was able to trace it as far as confirming that the "exit(1)" in packages/test-harness/lib/run-tests.js:runTests (around lind 70) is being called. But just after that, I see the "Total time:" and "Program terminated successfully" messages. So somehow that exit(1) isn't translating into an actual non-zero exit code. I don't yet know if "OK" is being written into the resultsfile or not.

Comment 2

[Brian Warner [:warner :bwarner]]

Irakli agreed to take this one one. Together, we were able to narrow it down to require("api-utils/system").exit() being called twice: once from run-tests.js:runTests (in the addon process), and again in response to a shutdown notification in bootstrap.js:shutdown(). Because these two calls are coming from different "processes" (in the e10s sense, actually they're both in a single OS process, but they're using two separate loader contexts), they get different instances of the system.js module, which means our first trick (a run-only-once flag in system.exit) didn't work (two distinct module instances means two distinct flags).

runTests() wants to call exit(0) or exit(1) to pass a result code back to the parent Python process, so it knows whether tests passed or failed. shutdown() wants to signal the parent Python process, so it knows that 'cfx run' has finished. Maybe there's a way to rearrange the functionality of exit() to make this work better? Like provide separate notifyResults() and notifyFinish() calls?

Irakli's latest idea is to have the addon-process-side code (in run-tests.js) obtain a remote reference to the chrome-process-side code, using the new e10s mechanisms, and ask it to call exit() on the chrome side, rather than calling exit() on the addon-process side.

Comment 3

[Brian Warner [:warner :bwarner]]

Attachment: pass CFX_COMMAND in runner environment

Irakli: this should add a CFX_COMMAND environment variable, from which the bootstrap-side code can figure out whether it's running as 'test' or 'run'. I'm pretty sure that aggregate commands like 'testall' or 'testpkgs' will result in CFX_COMMAND=test for each individual test, but it's worth verifying that assumption.

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/274

Pointer to Github pull-request

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 576640 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/274

Warner this change depends on you patch, and I'll test it once you'll fix patch so that it can be appiled to master.

Comment 6

[Brian Warner [:warner :bwarner]]

Attachment: pass CFX_COMMAND in runner environment (now against trunk)

oops, here's a patch that should apply cleanly against current trunk

Comment 7

[Brian Warner [:warner :bwarner]]

Comment on attachment 576640 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/274

other than the minor BAR/BAZ-could-be-even-less-likely comments, it looks good to me

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 576648 [details] [diff] [review]
pass CFX_COMMAND in runner environment (now against trunk)

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

Looks good!

Comment 9

[[github robot]]

Commits pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4c88c561fa41c700e4dc09361a4aceb9c4411f3c
Include patch for bug 704708 by @warner r=@gozala

https://github.com/mozilla/addon-sdk/commit/541cbed93f5837a0257efde75c828d41682c0767
Merge pull request #274 from Gozala/bug/exit-704708

fix Bug 704708 - failing test doesn't flunk 'cfx test' r=@warner