-
Notifications
You must be signed in to change notification settings - Fork 2
fix: #76 accumulate stderr #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: #76 accumulate stderr #75
Conversation
|
Not sure if this is related to CI, but I have been noticing intermittent issues in Greenwood test cases that seem related to this
Where one of the new reject based test cases keeps failing on the error output 1) Build Greenwood With:
Custom Configuration with a bad name value for a plugin
should throw an error that plugin.name is not a string:
AssertionError: expected promise to be rejected with an error including 'Configuration error: plugins must have a name. got undefined instead.' but got '\r\nNode.js v22.20.0\r\n'
+ expected - actual
-
-Node.js v22.20.0
+Configuration error: plugins must have a name. got undefined instead.I wonder if this will help? |
|
Lol, literally the same thing just happened on my new PR here 🙃 - #78 1) CLI Error Handling
handling (and bubbling) an exception from the child process
should throw an error that this is child throwing (a Promise.reject):
AssertionError: expected promise to be rejected with an error including 'Error: Child process throwing a Promi…' but got '\r\nNode.js v24.10.0\r\n'
+ expected - actual
-
-Node.js v24.10.0
+Error: Child process throwing a Promise.reject to the parent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind writing up an issue for this to detail the specifics (to superceed #76 ), for the release notes? I will also test this out in Greenwood tomorrow too, but I think this may be something we need for sure. 👍
|
|
||
| this.childProcess.stderr.on('data', (data) => { | ||
| err = data.toString('utf8'); | ||
| err += data.toString("utf8"); // Max string size ~1GiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, would this be the same thing though as we had before? I'm wondering if we need to concact the entire output before doing console.error? Or maybe I'm just misunderstanding, but this does feel like there is a there there, so to speak. 🤔
|
Got a PR up in Greenwood with this change and nothing is breaking! :) Will run it a couple more times but I think this one will be good to go once we clean up the title / details in the source issue. |
thescientist13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I understand what is happening now. Thanks for this fix!
I believe the cause of CI failing is that error messages are being overwritten by subsequent writes to
stderr. This PR fixes the failing CI by concatenating the stderr buffer rather than replacing it.Related Issue
Resolves #76
Summary of Changes
+=to buffer the stderr output