Skip to content

bpo-22674: fix test_strsignal on OSX#6085

Merged
pitrou merged 1 commit into
python:masterfrom
seirl:fix_test_strsignal
Mar 12, 2018
Merged

bpo-22674: fix test_strsignal on OSX#6085
pitrou merged 1 commit into
python:masterfrom
seirl:fix_test_strsignal

Conversation

@seirl

@seirl seirl commented Mar 12, 2018

Copy link
Copy Markdown
Contributor

We changed the behavior to remove OSX specific handling and forgot to update the tests. This should fix it, but I don't have an OSX machine so I can't test it myself... If the CI is still down for OSX, I'll need someone else to test it first :/

https://bugs.python.org/issue22674

Comment thread Lib/test/test_signal.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this could fail on other systems, I'd like the signal.strsignal result to be showed on failure. You can use the second argument of assertTrue for that.

@pitrou
pitrou requested a review from ned-deily March 12, 2018 16:38
@pitrou

pitrou commented Mar 12, 2018

Copy link
Copy Markdown
Member

Thanks. Hopefully a macOS user can come by and check this works...

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test no longer fails on macOS. Thanks!

@seirl
seirl force-pushed the fix_test_strsignal branch from 9fd4e0a to 65822fd Compare March 12, 2018 18:48
@seirl

seirl commented Mar 12, 2018

Copy link
Copy Markdown
Contributor Author

I used assertIn instead of assertTrue, it's a lot more explicit like that.

@pitrou

pitrou commented Mar 12, 2018

Copy link
Copy Markdown
Member

Thanks @seirl. @ned-deily, I expect that the test modification doesn't break macOS, as the new test is more lenient. Will merge.

@pitrou
pitrou merged commit 019f5b3 into python:master Mar 12, 2018
@seirl
seirl deleted the fix_test_strsignal branch March 12, 2018 19:04
jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants