Report errors in subprocesses #1102
Conversation
There was a problem hiding this comment.
Thanks for taking this up, @evmiguel!
A few of the new error messages present the current-working directory as though it were part of the command. It's good to provide the information as context, but we should probably describe it as distinct from the command which was executed.
client/tests/util/getPage.js
Outdated
| @@ -38,6 +38,20 @@ const startServer = async serverOrClient => { | |||
| } | |||
| ); | |||
|
|
|||
| server.on('error', error => { | |||
| console.info(`Error found in startServer process: ${error}`); // eslint-disable-line no-console | |||
There was a problem hiding this comment.
It seems as though the behavior of the tests will be meaningless from this moment forward, and the test runner will report that misleading error much more prominently than a stray console message. Shouldn't we interrupt test execution, instead?
There was a problem hiding this comment.
I agree. I've updated the code to exit on error.
server/scripts/import-tests/index.js
Outdated
|
|
||
| if (gitRunOutput.error) { | ||
| console.info( | ||
| `'git ${args.split(' ')} ${cwd}' failed with error ${ |
There was a problem hiding this comment.
Splitting on the empty space is necessary for spawn.sync. Here, though, by coercing the resultant array back into a string, we'll end up with a misrepresentation of the command executed (e.g. git fetch origin main becomes git fetch,origin,main).
There was a problem hiding this comment.
Thanks for catching this! I've updated the code to remove the split.
|
@jugglinmike Thank you for taking a look! I've removed the directories from the console messages and addressed your feedback. |
There was a problem hiding this comment.
Introducing process.exit to communicate an error gave me pause because that appeared to stop the test runner itself. Doing so wouldn't be the end of the world because we'd still be surfacing the problem along with some useful information (thanks to the use of console.info), but we'd also be interfering with the harness's other work (e.g. running other tests, performing cleanup logic, etc.).
I found some time to experiment today, and I learned that Jest appears to overwrite the built-in process.exit so that it can gracefully handle uses like this. However, I'd wager that it was implemented to handle invocations from the application under test. Using the overridden process.exit in the test environment may technically work, but it seems a bit misleading. I've rewritten the setup logic to throw an exception which will not be caught due to its origin in an event handler. Jest also accounts for this scenario, so the result is the same (the harness reports the error and continues with the rest of its work), but this way, the code doesn't insinuate that the process will immediately exit.
This PR addresses #957, which raised the issue of a lack of error reporting in subprocesses.