Skip to content

gh-130197: Improve test coverage of msgfmt.py #133048

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
merged 4 commits into from
May 1, 2025
Merged

gh-130197: Improve test coverage of msgfmt.py #133048

merged 4 commits into from
May 1, 2025

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Apr 27, 2025 •
edited by bedevere-app bot
Loading

I am using #130197 for this because it relates to #131381

The current msgfmt parser is not fully compliant and has several bugs despite a few having been fixed already. The parser code is also not very maintainable in its current form so I'd like to modernize it and make it more reliable. A reliable PO parser will also be useful for the --exclude-file option in pygettext (see #131381).

Before we make any sweeping changes, I want to first increase the test coverage which is currently really low. This will help us document the discrepancies of the current parser and ensure we don't regress when we update the parser later on.

This is just the first PR which adds string tests. More PRs will follow to test other parts of the PO parser.

@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Apr 27, 2025
@bedevere-app bedevere-app bot mentioned this pull request Apr 27, 2025
Open
18 tasks
@tomasr8
Copy link
Member Author

tomasr8 commented Apr 27, 2025

Do you think you could have a look Serhiy?

auvipy
auvipy approved these changes Apr 29, 2025
View reviewed changes
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

the tests looks good on first review

serhiy-storchaka
serhiy-storchaka reviewed May 1, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I'm afraid these tests take too much time (compared to their importance). A new process is started for each use case. Could you optimize them?

serhiy-storchaka
serhiy-storchaka approved these changes May 1, 2025
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@tomasr8
Copy link
Member Author

tomasr8 commented May 1, 2025

Could you optimize them?

Definitely, the overhead was about 3 seconds on my machine which is a lot. I changed it in a9bdce8 to import msgfmt directly instead of spawning a subprocess.

Note that we need to reset the global MESSAGES dictionary after each invocation since msgfmt keeps message data in a global variable.

The difference after the change is basically just random noise:

on main:

~ ./python Lib/test/test_tools/test_msgfmt.py
.............
----------------------------------------------------------------------
Ran 13 tests in 1.302s

on a9bdce8:

~ ./python Lib/test/test_tools/test_msgfmt.py                                                                                                                                                  
..............                                                                                                                                                                                                                                                
----------------------------------------------------------------------                                                                                                                                                                                        
Ran 14 tests in 1.305s

@tomasr8
Copy link
Member Author

tomasr8 commented May 1, 2025

LGTM.

Ah you are too fast 😆

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) May 1, 2025 13:12
@serhiy-storchaka serhiy-storchaka merged commit c73d460 into python:main May 1, 2025
42 checks passed
@tomasr8 tomasr8 deleted the msgfmt-parse-tests branch May 1, 2025 13:38
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD14 3.x (tier-3) has failed when building commit c73d460.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1232/builds/5601) and take a look at the build logs.
  4. Check if the failure is related to this commit (c73d460) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1232/builds/5601

Failed tests:

  • test_interpreters

Summary of the results of the build (if available):

==

Click to see traceback logs 
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1079, in _bootstrap_inner
    self._context.run(self.run)
    ~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/threading.py", line 1021, in run
    self._target(*self._args, **self._kwargs)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/test_interpreters/test_stress.py", line 30, in task
    interp = interpreters.create()
  File "/home/buildbot/buildarea/3.x.opsec-fbsd14/build/Lib/test/support/interpreters/__init__.py", line 76, in create
    id = _interpreters.create(reqrefs=True)
interpreters.InterpreterError: interpreter creation failed
k

@serhiy-storchaka serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label May 1, 2025
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

Sorry, @tomasr8 and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c73d46076ee0a6f32b472f9dfcf0e4245cc1c462 3.13

tomasr8 added a commit to tomasr8/cpython that referenced this pull request May 1, 2025
@tomasr8
[3.13] pythongh-130197: Improve test coverage of msgfmt.py (pythonGH-…
0c5b865 
…133048)

(cherry picked from commit c73d460)

Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented May 1, 2025

GH-133255 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 May 1, 2025
serhiy-storchaka pushed a commit that referenced this pull request May 2, 2025
@tomasr8
[3.13] gh-130197: Improve test coverage of msgfmt.py (GH-133048) (GH-…
ae775dc 
…133255)

(cherry picked from commit c73d460)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@auvipy auvipy auvipy approved these changes

Assignees

@serhiy-storchaka serhiy-storchaka

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
@tomasr8 @bedevere-bot @serhiy-storchaka @auvipy