-
Notifications
You must be signed in to change notification settings - Fork 37.8k
[tests] Allow tests to pass when stderr is non-empty #10703
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
Tests which pass with non-empty stderr are reported as "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.
utACK, a few nits.
@@ -40,17 +39,28 @@ | |||
CROSS = "x " | |||
CIRCLE = "o " | |||
|
|||
# Default colors to empty strings. | |||
BOLD, BLUE, RED, GREY, MAGENTA = [("", "")] * 5 |
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.
nit: alpha sort other than BOLD?
GREY = ('\033[0m', '\033[1;30m') | ||
RED = ('\033[0m', '\033[0;31m') | ||
BLUE = ('\033[0m', '\033[0;34m') | ||
MAGENTA = ('\033[0m', '\033[0;35m') |
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.
nit: alpha sort?
STATUS_PASSED_WITH_WARNINGS = "Passed with warnings" | ||
STATUS_SKIPPED = "Skipped" | ||
STATUS_FAILED = "Failed" | ||
STATUSES = [STATUS_PASSED, STATUS_PASSED_WITH_WARNINGS, STATUS_SKIPPED, STATUS_FAILED] |
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.
This doesn't seem to be used anywhere else, maybe combine with line below?
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.
IMO its cleaner how it is, shouldn't be combined
elif self.status == "Skipped": | ||
if self.status == STATUS_PASSED_WITH_WARNINGS: | ||
color = MAGENTA | ||
glyph = TICK |
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.
Is TICK the best glyph to use for passed with warnings? Or something more neutral?
utACK d64ac3f |
The code here looks like mostly output formatting changes, not actually dealing with the issue described by the title? |
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.
utACK d64ac3f. Note that #10882 is currently depending on this so it needs 0.15 milestone.
The code here looks like mostly output formatting changes, not actually dealing with the issue described by the title?
"Allow tests to pass" implies output formatting has to change at least somewhat to avoid giving impression that tests failed when they actually succeeded, and these changes are a pretty straightforward way of updating the output.
STATUS_FAILED = "Failed" | ||
STATUSES = [STATUS_PASSED, STATUS_PASSED_WITH_WARNINGS, STATUS_SKIPPED, STATUS_FAILED] | ||
|
||
STATUS_MAX_LEN = max([len(st) for st in STATUSES]) |
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.
Square brackets unneeded and probably should be dropped for slightly better readability & efficiency.
utACK d64ac3f |
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
Reverted this in 60f9778 after discussion on IRC. |
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.