-
Notifications
You must be signed in to change notification settings - Fork 29k
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
tools: show stdout/stderr for timed out tests #20260
Conversation
When a test times out, the contents of stdout and stderr can often be highly valuable in debugging. Provide that information. Refs: nodejs#19906 (comment)
@nodejs/build @nodejs/testing |
@@ -333,7 +333,7 @@ def HasRun(self, output): | |||
(total_seconds, duration.microseconds / 1000)) | |||
if self.severity is not 'ok' or self.traceback is not '': | |||
if output.HasTimedOut(): | |||
self.traceback = 'timeout' | |||
self.traceback = 'timeout\n' + output.output.stdout + output.output.stderr |
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.
I suggest to add a line break between stdout and stderr.
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.
The way it's done here is the way it is done on line 291. Wanted to keep it consistent. Maybe a subsequent PR can change them both if there's agreement that a blank line would help.
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.
LGTM
(Please 👍 for fast-tracking. Thanks!) |
@Trott I personally believe that is already "accepted" due to adding the label earlier and getting a couple LGs later on without anyone saying something against it. That is at least how I understood, that we handled it so far. |
@@ -333,7 +333,7 @@ def HasRun(self, output): | |||
(total_seconds, duration.microseconds / 1000)) | |||
if self.severity is not 'ok' or self.traceback is not '': | |||
if output.HasTimedOut(): | |||
self.traceback = 'timeout' | |||
self.traceback = 'timeout\n' + output.output.stdout + output.output.stderr |
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.
After reviewing #20295, do we expect anything in stderr/stdout if output.UnexpectedOutput()
on L287 is false? If not can we simplify to self.traceback += '\ntimeout'
as self.traceback
is set on L291 with the contents of stderr/stdout.
@BridgeAR I agree that's how people have been handling it, but I'm of the opinion that is in contradiction with the documented process, which at least implies that approval of fast-tracking should be explicit and not implicit like that. I've been pondering whether the documented process should be clarified to conform with current practice or if we should push for explicit fast-track approval. Until then, I intend to request explicit approval when I want to fast-track. |
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.
-1 to fast track
+1 for change, but IMHO this should be regular-tracked (RE: #19855 (comment)) /CC @rvagg |
This is one area that we can't fully test the consequences of the change and may be surprised by the results. I don't know if that should impact fast-tracking or not, just a word of warning that we often find odd dependencies on output like this and we won't really know until we break it—or we just try not to break it by being extra care with our changes (e.g. like my objection to |
When a test times out, the contents of stdout and stderr can often be highly valuable in debugging. Provide that information. Refs: nodejs#19906 (comment) PR-URL: nodejs#20260 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Landed in 169756b |
When a test times out, the contents of stdout and stderr can often be highly valuable in debugging. Provide that information. Refs: #19906 (comment) PR-URL: #20260 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When a test times out, the contents of stdout and stderr can often be highly valuable in debugging. Provide that information. Refs: #19906 (comment) PR-URL: #20260 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
When a test times out, the contents of stdout and stderr can often be
highly valuable in debugging. Provide that information.
Refs: #19906 (comment)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes