Skip to content

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

Merged
serhiy-storchaka merged 1 commit into
python:mainfrom
serhiy-storchaka:test-gh-108654
Sep 11, 2023
Merged

gh-109182: Fix and improve tests for gh-108654#109189
serhiy-storchaka merged 1 commit into
python:mainfrom
serhiy-storchaka:test-gh-108654

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented Sep 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka added tests Tests in the Lib/test dir needs backport to 3.12 only security fixes labels Sep 9, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 9, 2023
Closed
@serhiy-storchaka serhiy-storchaka mentioned this pull request Sep 10, 2023
Closed
carljm
carljm approved these changes Sep 11, 2023
View reviewed changes

@carljm carljm 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.

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

serhiy-storchaka commented Sep 11, 2023

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

serhiy-storchaka commented Sep 11, 2023

Copy link
Copy Markdown
Member Author

Thank you for your review @carljm.

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

miss-islington commented Sep 11, 2023

Copy link
Copy Markdown
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
@serhiy-storchaka @miss-islington
pythongh-109182: Fix and improve tests for pythongh-108654 (pythonGH-…
d03b272 
…109189)

(cherry picked from commit c0f488b)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app

bedevere-app Bot commented Sep 11, 2023

Copy link
Copy Markdown

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

carljm commented Sep 11, 2023

Copy link
Copy Markdown
Member

_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
@miss-islington @serhiy-storchaka
[3.12] gh-109182: Fix and improve tests for gh-108654 (GH-109189) (#1…
5305cdc 
…09271)

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

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@serhiy-storchaka @miss-islington @carljm @bedevere-bot