fix(core): ensure verbose logs go to stderr #34358
Conversation
|
View your CI Pipeline Execution ↗ for commit 200d954
☁️ Nx Cloud last updated this comment at |
cb05f0c to
cc3fd9e
Compare
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| it('should handle pipe errors gracefully', (done) => { | ||
| const errorStream = new Writable({ | ||
| write: (chunk, encoding, callback) => { | ||
| callback(new Error('Write failed')); | ||
| }, | ||
| }); | ||
|
|
||
| const transformer = daemonStreamTransformer(errorStream); | ||
| const readable = Readable.from(['line1\n']); | ||
|
|
||
| readable.on('end', () => { | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test doesn't validate error handling: The test creates a readable stream but never pipes it to the transformer. The 'end' event fires immediately without testing the error scenario.
The test should actually pipe the streams:
readable.pipe(transformer).on('error', (err) => {
// Verify error was handled
done();
});Currently, this test passes but doesn't validate the intended error handling behavior.
| it('should handle pipe errors gracefully', (done) => { | |
| const errorStream = new Writable({ | |
| write: (chunk, encoding, callback) => { | |
| callback(new Error('Write failed')); | |
| }, | |
| }); | |
| const transformer = daemonStreamTransformer(errorStream); | |
| const readable = Readable.from(['line1\n']); | |
| readable.on('end', () => { | |
| done(); | |
| }); | |
| }); | |
| it('should handle pipe errors gracefully', (done) => { | |
| const errorStream = new Writable({ | |
| write: (chunk, encoding, callback) => { | |
| callback(new Error('Write failed')); | |
| }, | |
| }); | |
| const transformer = daemonStreamTransformer(errorStream); | |
| const readable = Readable.from(['line1\n']); | |
| readable.pipe(transformer).on('error', (err) => { | |
| // Error was handled, test passes | |
| done(); | |
| }); | |
| }); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
cd9ac13 to
99a9923
Compare
| bodyLines.forEach( | ||
| (bodyLine) => this.writeToStream(`${bodyLine}${EOL}`), | ||
| stream | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incorrect usage of forEach - the stream parameter is being passed as the second argument which is thisArg, not as a parameter to writeToStream. This will cause bodyLines to be written to process.stdout (the default) instead of the intended stream.
// Fix:
bodyLines.forEach((bodyLine) => this.writeToStream(`${bodyLine}${EOL}`, stream));| bodyLines.forEach( | |
| (bodyLine) => this.writeToStream(`${bodyLine}${EOL}`), | |
| stream | |
| ); | |
| } | |
| bodyLines.forEach( | |
| (bodyLine) => this.writeToStream(`${bodyLine}${EOL}`, stream) | |
| ); | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
cf8fe8e to
a22babf
Compare
2fb81d2 to
8103a2e
Compare
60fcd64 to
4572e6e
Compare
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Failed tasks: e2e-nx, e2e-release e2e tests Local verification: e2e-only (applied self-healing fix)
…ests Move stderr combination from shell-level 2>&1 to Node.js error handler, and add redirectStderr: true to cache and version-plans-filtered-release tests. Failed tasks: e2e-nx, e2e-release e2e tests Local verification: enhanced self-healing fix
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Before 2>&1 was removed, execSync's return value naturally contained stderr via shell redirection when redirectStderr was true. After the move to Node-level handling, stderr is dropped on the success path because execSync returns only stdout. Additionally, switching stdio[2] to 'inherit' by default meant that e.stderr was null on thrown errors, breaking tests that inspect it. Restore stdio: 'pipe' so e.stderr is populated on throw, and append 2>&1 at the shell level when redirectStderr is true so combined output is captured on success. Add redirectStderr: true to the e2e-release tests that assert on NX error/warning text routed to stderr.
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Per review feedback, flip the default so e2e tests no longer have to opt in per-call to see stderr output. The 2>&1 shell redirect already merges streams into stdout, so the silenceError branch simplifies to just returning e.stdout. Revert the scattered redirectStderr: true additions across the e2e suite and update extras.test.ts to read the merged output from e.stdout instead of e.stderr.
Defaulting redirectStderr to true broke tests that rely on clean stdout (JSON.parse on show project --json) and widened many existing snapshot assertions with stderr content that wasn't there on master. Revert the default to undefined. On the silenceError error path, default to concatenating e.stdout + e.stderr so tests that expect error text still get it without opting in explicitly.
With redirectStderr no longer defaulting to true, tests that inspect messages emitted via output.warn (stderr) or output.error (stderr) need to opt in again: - workspace.test.ts: read error.stderr instead of error.stdout since the "still a dependency" error is thrown and surfaced via output.error. - version-plans-check.test.ts: pass redirectStderr:true on the two verbose plan:check calls that assert on the "No changed files found" warning and the NX_BASE/NX_HEAD note wrappers. - version-plans-only-touched.test.ts: pass redirectStderr:true on the no-changed-projects call whose snapshot includes the "No version bumps were selected" warning.
…tput ordering The release plan:check output now emits all 'Touched projects based on changed files' blocks before the 'Touched projects missing version plans' error blocks, rather than interleaving them per group.
1b32651 to
01e632a
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated the assertion in extras.test.ts to check e.stderr instead of e.stdout for the "command exited with non-zero status code" message. This fixes the failure because the PR routes output.warn to stderr, so the warning no longer appears in stdout. Aligning the assertion with the new stream routing restores the test's correctness.
Note
⏳ We are verifying this fix by re-running e2e-nx:e2e-ci--src/extras.test.ts.
diff --git a/e2e/nx/src/extras.test.ts b/e2e/nx/src/extras.test.ts
index 7a1d63a317..55d2496352 100644
--- a/e2e/nx/src/extras.test.ts
+++ b/e2e/nx/src/extras.test.ts
@@ -312,7 +312,7 @@ describe('Extra Nx Misc Tests', () => {
runCLI(`run ${mylib}:error`);
fail('Should error if process errors');
} catch (e) {
- expect(e.stdout.toString()).toContain(
+ expect(e.stderr.toString()).toContain(
'command "exit 1" exited with non-zero status code'
);
}
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally 3I9y-9qZu
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated the assertion in extras.test.ts to check e.stderr instead of e.stdout for the "command exited with non-zero status code" message. This fixes the failure because the PR routes output.warn to stderr, so the warning no longer appears in stdout. Aligning the assertion with the new stream routing restores the test's correctness.
Tip
✅ We verified this fix by re-running e2e-nx:e2e-ci--src/extras.test.ts.
diff --git a/e2e/nx/src/extras.test.ts b/e2e/nx/src/extras.test.ts
index 7a1d63a317..55d2496352 100644
--- a/e2e/nx/src/extras.test.ts
+++ b/e2e/nx/src/extras.test.ts
@@ -312,7 +312,7 @@ describe('Extra Nx Misc Tests', () => {
runCLI(`run ${mylib}:error`);
fail('Should error if process errors');
} catch (e) {
- expect(e.stdout.toString()).toContain(
+ expect(e.stderr.toString()).toContain(
'command "exit 1" exited with non-zero status code'
);
}
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally 3I9y-9qZu
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
There was a problem hiding this comment.
✅ The fix from Nx Cloud was applied
We updated the assertion in extras.test.ts to check e.stderr instead of e.stdout for the "command exited with non-zero status code" message. This fixes the failure because the PR routes output.warn to stderr, so the warning no longer appears in stdout. Aligning the assertion with the new stream routing restores the test's correctness.
Tip
✅ We verified this fix by re-running e2e-nx:e2e-ci--src/extras.test.ts.
Suggested Fix changes
diff --git a/e2e/nx/src/extras.test.ts b/e2e/nx/src/extras.test.ts
index 7a1d63a317..55d2496352 100644
--- a/e2e/nx/src/extras.test.ts
+++ b/e2e/nx/src/extras.test.ts
@@ -312,7 +312,7 @@ describe('Extra Nx Misc Tests', () => {
runCLI(`run ${mylib}:error`);
fail('Should error if process errors');
} catch (e) {
- expect(e.stdout.toString()).toContain(
+ expect(e.stderr.toString()).toContain(
'command "exit 1" exited with non-zero status code'
);
}
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
View interactive diff ↗➡️ This fix was applied by Craigory Coppola
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
✅ The fix from Nx Cloud was applied
We updated the assertion in extras.test.ts to check e.stderr instead of e.stdout for the "command exited with non-zero status code" message. This fixes the failure because the PR routes output.warn to stderr, so the warning no longer appears in stdout. Aligning the assertion with the new stream routing restores the test's correctness.
Tip
✅ We verified this fix by re-running e2e-nx:e2e-ci--src/extras.test.ts.
Suggested Fix changes
diff --git a/e2e/nx/src/extras.test.ts b/e2e/nx/src/extras.test.ts
index 7a1d63a317..55d2496352 100644
--- a/e2e/nx/src/extras.test.ts
+++ b/e2e/nx/src/extras.test.ts
@@ -312,7 +312,7 @@ describe('Extra Nx Misc Tests', () => {
runCLI(`run ${mylib}:error`);
fail('Should error if process errors');
} catch (e) {
- expect(e.stdout.toString()).toContain(
+ expect(e.stderr.toString()).toContain(
'command "exit 1" exited with non-zero status code'
);
}
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
View interactive diff ↗➡️ This fix was applied by Craigory Coppola
🎓 Learn more about Self-Healing CI on nx.dev
The daemonStreamTransformer, formatLogMessage helper, DaemonLogger export, and companion spec file were never consumed by the stderr routing work and were blocking review. Revert packages/nx/src/daemon/logger.ts to master and delete the daemon spec.
| color: string; | ||
| title: string; | ||
| }, | ||
| stream: WriteStream = process.stdout |
There was a problem hiding this comment.
Make it so you always have to pass this in... it actually seems like this is only called with stderr
| private writeOptionalOutputBody(bodyLines?: string[]): void { | ||
| private writeOptionalOutputBody( | ||
| bodyLines?: string[], | ||
| stream: WriteStream = process.stdout |
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
logger.[x]doesn't get decorated despite being emitted to the daemon log, perf logs aren't decorated, etcExpected Behavior
daemon logs are decorated
Related Issue(s)
Fixes #