Skip to content

gh-130655: Add tests for gettext.find() #130691

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
Merged
encukou merged 12 commits into from
Mar 19, 2025
Merged

gh-130655: Add tests for gettext.find() #130691

encukou merged 12 commits into from
Mar 19, 2025

Conversation

@StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented Feb 28, 2025 •
edited
Loading

(There was no coverage before)

image

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels Feb 28, 2025
@bedevere-app bedevere-app bot mentioned this pull request Feb 28, 2025
Open
@tomasr8 tomasr8 self-requested a review February 28, 2025 18:40
@StanFromIreland
Copy link
Member Author

StanFromIreland commented Feb 28, 2025

Also request @serhiy-storchaka

@tomasr8
Copy link
Member

tomasr8 commented Feb 28, 2025

Thanks! Could you also add tests that cover the 4 missing lines?

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Feb 28, 2025

First line: I would consider fine as it just breaks on the first run

Second line: not sure why it’s yellow

third line: Is it necessary?

fourth line: returns before reaching c

I don’t see any reason to run a test with just c

@tomasr8
Copy link
Member

tomasr8 commented Feb 28, 2025

It's important to test these if we're going to touch the behaviour of find later. The function is quite complex and I wouldn't feel confident modifying it w/o proper test coverage. Since we're going through the trouble to write tests for it, let's make sure it's all covered :)

First line: I would consider fine as it just breaks on the first run

The loop always breaks which means you never test the case where languages == [].

Second line: not sure why it’s yellow

The condition was always True in tests

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Feb 28, 2025

100% (of find)

image

encukou
encukou reviewed Mar 3, 2025
View reviewed changes
@StanFromIreland StanFromIreland requested a review from encukou March 3, 2025 19:10
tomasr8
tomasr8 reviewed Mar 3, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Just a few comments/improvements :)

@StanFromIreland StanFromIreland requested a review from tomasr8 March 3, 2025 21:09
@tomasr8
Copy link
Member

tomasr8 commented Mar 10, 2025

Can you double-check that we have full coverage? Otherwise I think it's good :)

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Mar 10, 2025

@tomasr8

Looks pretty green to me :-)

report

image

tomasr8
tomasr8 reviewed Mar 10, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Just one more thing, I'd also test that LC_ALL, LC_MESSAGES and LANG are read as well in addition to LANGUAGE.

@StanFromIreland StanFromIreland requested a review from tomasr8 March 10, 2025 19:08
tomasr8
tomasr8 approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good :)

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 18, 2025
@bedevere-bot
Copy link

bedevere-bot commented Mar 18, 2025

🤖 New build scheduled with the buildbot fleet by @encukou for commit 9afb57d 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130691%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 18, 2025
@encukou encukou merged commit 3118693 into python:main Mar 19, 2025
113 of 115 checks passed
@StanFromIreland StanFromIreland deleted the find-tests branch March 19, 2025 15:25
colesbury pushed a commit to colesbury/cpython that referenced this pull request Mar 20, 2025
@StanFromIreland @tomasr8 @colesbury
pythongh-130655: Add tests for gettext.find() (pythonGH-130691)
09a9cfd 
Co-authored-by: Tomas R. <[email protected]>
@picnixz
Copy link
Member

picnixz commented Apr 4, 2025

Do we want to backport them or not by the way?

@tomasr8
Copy link
Member

tomasr8 commented Apr 4, 2025

I think so, similar test PRs were also previously backported.

@tomasr8 tomasr8 added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Apr 4, 2025
@miss-islington-app
Copy link

miss-islington-app bot commented Apr 4, 2025

Thanks @StanFromIreland for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Apr 4, 2025

Thanks @StanFromIreland for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 4, 2025
@StanFromIreland @tomasr8 @miss-islington
pythongh-130655: Add tests for gettext.find() (pythonGH-130691)
04b2b6d 
(cherry picked from commit 3118693)

Co-authored-by: Stan Ulbrych <[email protected]>
Co-authored-by: Tomas R. <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 4, 2025
@StanFromIreland @tomasr8 @miss-islington
pythongh-130655: Add tests for gettext.find() (pythonGH-130691)
c6755ec 
(cherry picked from commit 3118693)

Co-authored-by: Stan Ulbrych <[email protected]>
Co-authored-by: Tomas R. <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Apr 4, 2025

GH-132083 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 Apr 4, 2025
@bedevere-app
Copy link

bedevere-app bot commented Apr 4, 2025

GH-132084 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Apr 4, 2025
picnixz pushed a commit that referenced this pull request Apr 4, 2025
@miss-islington @StanFromIreland @tomasr8
[3.12] gh-130655: Add tests for gettext.find() (GH-130691) (#132083)
65a0103 
gh-130655: Add tests for `gettext.find()` (GH-130691)

(cherry picked from commit 3118693)

Co-authored-by: Stan Ulbrych <[email protected]>
Co-authored-by: Tomas R. <[email protected]>
picnixz pushed a commit that referenced this pull request Apr 4, 2025
@miss-islington @StanFromIreland @tomasr8
[3.13] gh-130655: Add tests for gettext.find() (GH-130691) (#132084)
093c587 
gh-130655: Add tests for `gettext.find()` (GH-130691)

(cherry picked from commit 3118693)

Co-authored-by: Stan Ulbrych <[email protected]>
Co-authored-by: Tomas R. <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 approved these changes

@encukou encukou Awaiting requested review from encukou

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.

5 participants

@StanFromIreland @tomasr8 @bedevere-bot @picnixz @encukou