Skip to content

gh-105639: Fix reference_find test for empty strings #105640

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

gh-105639: Fix reference_find test for empty strings #105640

wants to merge 6 commits into from

Conversation

sweeneyde
Copy link
Member

@sweeneyde sweeneyde commented Jun 10, 2023 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot mentioned this pull request Jun 10, 2023
Closed
@sweeneyde sweeneyde marked this pull request as ready for review June 10, 2023 22:29
@sweeneyde sweeneyde added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes tests Tests in the Lib/test dir labels Jun 10, 2023
@sweeneyde sweeneyde changed the title gh-105639: Fix reference_find for empty strings gh-105639: Fix reference_find test for empty strings Jun 10, 2023
terryjreedy
terryjreedy requested changes Jun 11, 2023
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

The test should deterministically test ('','') and perhaps some other corner-case pairs in another loop.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@sweeneyde
Copy link
Member Author

sweeneyde commented Jun 11, 2023 •
edited
Loading

The test should deterministically test ('','') and perhaps some other corner-case pairs in another loop.

There is already a test_find a few methods above:

cpython/Lib/test/string_tests.py

Line 180 in 3a314f7

self.checkequal(0, '', 'find', '')

(Edit: I went ahead and added some deterministic cases as well to test that str.find and reference_find agree in the simple cases as well.)

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from terryjreedy June 11, 2023 06:36
@sweeneyde
Copy link
Member Author

@terryjreedy, is this better now?

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.11 only security fixes labels May 9, 2024
@hugovk
Copy link
Member

hugovk commented Mar 7, 2025

Thanks for the PR, however it's been subsequently resolved by PR #110170 and issue #110160.

@hugovk hugovk closed this Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pochmann pochmann pochmann left review comments

@terryjreedy terryjreedy terryjreedy approved these changes

Assignees
No one assigned
Labels
awaiting merge needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sweeneyde @bedevere-bot @hugovk @pochmann @terryjreedy @serhiy-storchaka