-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Allow tests to pass even when stderr got populated #10241
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
Conversation
|
I thought about this a bit: apparently some people use the stderr output to determine whether a test emitted e.g. sanitizer warnings. |
…sed with warnings"
7e0f99f to
41f392c
Compare
|
Added a new status "Passed with warnings" which gets set when the test passed but stderr was not empty. The test will be flagged as "successful" for that new status. |
| color = BLUE | ||
| glyph = TICK | ||
| if self.status == "Passed with warnings": | ||
| color = ORANGE |
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.
NameError: name 'ORANGE' is not defined
jnewbery
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.
Couple of comments inline.
If you add a new status string, you'll need to update print justification (otherwise I think your new category will just be truncated). Justification is on L337 in print_results() and L422 in TestResult.__repr__().
Now that we have 4 categories, it may make sense to define all the categories as constants at the top of the file and have a new MAX_STATUS_WIDTH constant as well.
One you've done that, can you add a screenshot of what this looks like?
| color = BLUE | ||
| glyph = TICK | ||
| if self.status == "Passed with warnings": | ||
| color = ORANGE |
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.
ORANGE would need to be a constant defined at the top of this file (see around L47)
| if self.status == "Passed": | ||
| color = BLUE | ||
| glyph = TICK | ||
| if self.status == "Passed with warnings": |
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.
elif please
|
Concept ACK, but I will close this pull in case no one picks it up in the next couple of days. |
|
Closing for now. Feel free to pick this up again. |
d64ac3f [tests] Allow tests to pass when stderr is non-empty (Jonas Schnelli) Pull request description: Resurrect #10241 with nits addressed Not sure how much people want this. Would be useful for functional tests which cause bitcoind to print to stderr. Tree-SHA512: 28caccf7818fb3ed5a38caef7f77161b1678aa9b8fd12c2d1e76032f409f0d33c40f7ac91e0c8d908df4a44fd01cf97d657a08bae50c6ff17d07f5b2e20c3a6e
No description provided.