Add test.debug to integration tests
#14133
Conversation
| } | ||
|
|
||
| options.onTestFinished(dispose) | ||
|
|
||
| let context = { |
There was a problem hiding this comment.
This was moved up so that pnpm install can use the same exec function that we also export
| @@ -177,12 +147,14 @@ export function test( | |||
|
|
|||
| child.stdout.on('data', (result) => { | |||
| let content = result.toString() | |||
| if (debug) console.log(content) | |||
There was a problem hiding this comment.
Should this have like a [stdout] prefix or something? Just so it's more easily distinguishable in the test output?
There was a problem hiding this comment.
thinking out loud: should probably prefix every line of the output
There was a problem hiding this comment.
Hmm, maybe? It could help with the onStderr helper we have but I wonder why we need to be so diligent about this in the first place. In normal terminal apps you don't see the difference between stdout and stderr in the console either unless you pipe line stream to another location. I decided to replicate this here by using console.log for stdout and console.error for stderr
There was a problem hiding this comment.
I personally don't think the [stdout] or [stderr] prefix helps here because Vitest already shows that for us:
| combined.push(['stdout', content]) | ||
| stdoutMessages.push(content) | ||
| notifyNext(stdoutActors, stdoutMessages) | ||
| }) | ||
| child.stderr.on('data', (result) => { | ||
| let content = result.toString() | ||
| if (debug) console.error(content) |
There was a problem hiding this comment.
Ditto but with [stderr]
| @@ -177,12 +147,14 @@ export function test( | |||
|
|
|||
| child.stdout.on('data', (result) => { | |||
| let content = result.toString() | |||
| if (debug) console.log(content) | |||
There was a problem hiding this comment.
I personally don't think the [stdout] or [stderr] prefix helps here because Vitest already shows that for us:
| @@ -127,6 +96,7 @@ export function test( | |||
| rejectDisposal = reject | |||
| }) | |||
|
|
|||
| if (debug) console.log(`>& ${command}`) | |||
There was a problem hiding this comment.
We could do the same here as above, but this now results in the same command log, but just with a different prefix:
I would maybe just drop this? 🤔
| if (debug) console.log(`>& ${command}`) |
There was a problem hiding this comment.
@RobinMalfait I’m confused, how did you get to the above log? Were you doing both exec() and spawn()? (and if that is the case, would you not expect both to log? 🤔)
There was a problem hiding this comment.
Oh wait, right, I'm confused as well haha. Not sure how I got the output twice. Please ignore.
RobinMalfait
force-pushed
the
feat/add-content-support
branch
from
ad400be
to
737a5d2
Compare
|
I think this PR should be merged in |
|
@RobinMalfait yeah I need to rebase it I just wanted it to be based on the latest changes :) |
Co-authored-by: Robin Malfait <malfait.robin@gmail.com>
philipp-spiess
force-pushed
the
feat/test.debug
branch
from
f893850
to
10c7868
Compare
philipp-spiess
force-pushed
the
feat/test.debug
branch
from
499d478
to
10c7868
Compare
While working on #14078, there were a couple of debugging techniques that we were using quite frequently:
cdinto the test setupSince we naturally worked around this quite often, I decided to make this a first-level API with the introduction of a new
test.debugflag. When set, it will:.debugfolder and won't delete it after the run. Having it in an explicit folder allows us to easily delete it manually when we need to.>for a sync call,>&for a spawned process).only