test: improve test error message logs #23694
Conversation
| @@ -96,39 +96,47 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<Proce | |||
| _processes.push(childProcess); | |||
|
|
|||
| // Create the error here so the stack shows who called this function. | |||
There was a problem hiding this comment.
Seems like this wasn't actually done before? Creating the error with the stack trace at this point is the primary change. Otherwise the stack trace is always just the child_process exit/error stack which is useless.
| // Return log info about the current process status | ||
| function envDump() { | ||
| return [ | ||
| `ENV:${JSON.stringify(spawnOptions.env, null, 2)}`, |
There was a problem hiding this comment.
This also changed but is a bit hard to see. It now outputs the env of the process being spawned, not the parent process - spawnOptions.env isntead of process.env. I think that is far more useful and probably the original intention?
| @@ -96,39 +96,47 @@ function _exec(options: ExecOptions, cmd: string, args: string[]): Promise<Proce | |||
| _processes.push(childProcess); | |||
|
|
|||
| // Create the error here so the stack shows who called this function. | |||
| let error: Error; | |||
There was a problem hiding this comment.
Wouldn't the below do the same?
| let error: Error; | |
| const error = new Error(); |
There was a problem hiding this comment.
Looks like you're correct. I always thought the stack was where the error was thrown, but it's actually just where it was constructed?
There was a problem hiding this comment.
That is correct. Can also change the catch to which is more readable.
}).catch((err) => {
// Create the error here so the stack shows who called this function.
return Promise.reject(new Error(err));
});There was a problem hiding this comment.
If you do the new Error in the catch callback doesn't that mean the stack trace will be just the callback?
jbedard
force-pushed
the
e2e-error-stacks
branch
2 times, most recently
from
f1cdb91 to
e5c5c4e
Compare
jbedard
force-pushed
the
e2e-error-stacks
branch
from
e5c5c4e to
320f6ea
Compare
jbedard
force-pushed
the
e2e-error-stacks
branch
from
320f6ea to
ac9a6c8
Compare
| reject( | ||
| `Process output didn't match - "${cmd} ${args.join(' ')}": '${ | ||
| options.waitForMatch | ||
| }': ${code}...\n\n${envDump()}\n`, |
There was a problem hiding this comment.
Note both the code (formerly error) and the envDump() were not here previously.
alan-agius4
added
target: patch
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.