Skip to content

gh-109182: Fix and improve tests for gh-108654 #109189

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

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

I prefer to keep the use of with self.subTest(...) when a test case is checking two related cases. It's not a huge deal, but it keeps the two cases independent, so that if the first one fails, the second one still runs instead of being silently skipped, so that a single run of the tests can give a fuller picture of the state of the system under test.

Otherwise, these changes look like clear improvements to the comprehensiveness (and readability) of the tests. Thank you!

@serhiy-storchaka
Copy link
Member Author

_check_in_scopes() uses subTest() internally. So 2 subsequent _check_in_scopes()s can give you up to 6 failure reports (3 reports each).

@serhiy-storchaka
Copy link
Member Author

Thank you for your review @carljm.

@serhiy-storchaka serhiy-storchaka merged commit c0f488b into python:main Sep 11, 2023
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the test-gh-108654 branch September 11, 2023 14:50
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 11, 2023

GH-109271 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 11, 2023
@carljm
Copy link
Member

carljm commented Sep 11, 2023

_check_in_scopes() uses subTest() internally. So 2 subsequent _check_in_scopes()s can give you up to 6 failure reports (3 reports each).

Oh, good point! So we already will get independent failures for all six cases. The only thing we lose by eliminating the outer subTest is the extra raises context value -- but by eliminating the loop, we no longer need that, since it is effectively provided by the line number.

Ok, then I am happy with these changes as is!

Yhg1s pushed a commit that referenced this pull request Sep 12, 2023
…09271)

gh-109182: Fix and improve tests for gh-108654 (GH-109189)
(cherry picked from commit c0f488b)

Co-authored-by: Serhiy Storchaka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants