Skip to content

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

Closed
sweeneyde wants to merge 6 commits into
python:mainfrom
sweeneyde:find_periodic_test
Closed

gh-105639: Fix reference_find test for empty strings#105640
sweeneyde wants to merge 6 commits into
python:mainfrom
sweeneyde:find_periodic_test

Conversation

@sweeneyde

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

Copy link
Copy Markdown
Member

@bedevere-botbedevere-bot mentioned this pull request Jun 10, 2023
Closed
@sweeneyde
sweeneyde marked this pull request as ready for review June 10, 2023 22:29
@sweeneydesweeneyde 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
@sweeneydesweeneyde changed the title gh-105639: Fix reference_find for empty stringsgh-105639: Fix reference_find test for empty stringsJun 10, 2023
terryjreedy
terryjreedy requested changes Jun 11, 2023
View reviewed changes

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

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

Comment threadLib/test/string_tests.py Outdated
@bedevere-bot

bedevere-bot commented Jun 11, 2023

Copy link
Copy Markdown

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

@sweeneyde

sweeneyde commented Jun 11, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

bedevere-bot commented Jun 11, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

@sweeneyde

sweeneyde commented Aug 22, 2023

Copy link
Copy Markdown
MemberAuthor

@terryjreedy, is this better now?

@serhiy-storchakaserhiy-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

hugovk commented Mar 7, 2025

Copy link
Copy Markdown
Member

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

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

Reviewers

@terryjreedyterryjreedyterryjreedy approved these changes
+1 more reviewer
@pochmannpochmannpochmann left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting mergeneeds backport to 3.12only security fixesneeds backport to 3.13bugs and security fixestestsTests 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