355 bytes, text/html
2.26 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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.
Assignee: nobody → rFobic
Created attachment 576561 [details] [diff] [review] 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.
Created attachment 576640 [details] Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/274 Pointer to Github pull-request
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.
Attachment #576640 - Flags: review?(warner-bugzilla)
Created attachment 576648 [details] [diff] [review] pass CFX_COMMAND in runner environment (now against trunk) oops, here's a patch that should apply cleanly against current trunk
Attachment #576561 - Attachment is obsolete: true
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
Attachment #576640 - Flags: review?(warner-bugzilla) → review+
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!
Attachment #576648 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.