Skip to content

Conversation

jnewbery
Copy link
Contributor

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.

Tests which pass with non-empty stderr are reported as "passed with
warnings"
Copy link
Contributor

@jimmysong jimmysong left a 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
Copy link
Contributor

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')
Copy link
Contributor

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]
Copy link
Contributor

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?

Copy link
Contributor

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
Copy link
Contributor

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?

@meshcollider
Copy link
Contributor

meshcollider commented Jul 1, 2017

utACK d64ac3f

@sipa
Copy link
Member

sipa commented Jul 25, 2017

The code here looks like mostly output formatting changes, not actually dealing with the issue described by the title?

Copy link
Contributor

@ryanofsky ryanofsky left a 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])
Copy link
Contributor

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.

@maflcko
Copy link
Member

maflcko commented Jul 25, 2017

utACK d64ac3f

@maflcko maflcko merged commit d64ac3f into bitcoin:master Jul 25, 2017
maflcko pushed a commit that referenced this pull request Jul 25, 2017
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
@laanwj
Copy link
Member

laanwj commented Jul 25, 2017

Reverted this in 60f9778 after discussion on IRC.
Sorry @MarcoFalke but there seems no agreement on doing this change.
It's unfortunate that #10882 requires this, but it seems like we'll have to look for an alternative solution there.

@jnewbery jnewbery deleted the test_stderr branch July 26, 2017 07:41
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants