Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-130197: Improve test coverage ofmsgfmt.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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Do you think you could have a look Serhiy? |
auvipy left a comment
There was a problem hiding this 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
There was a problem hiding this 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?
This makes the tests much faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM.
Definitely, the overhead was about 3 seconds on my machine which is a lot. I changed it ina9bdce8 to import Note that we need to reset the global The difference after the change is basically just random noise: on main: ~ ./pythonLib/test/test_tools/test_msgfmt.py.............----------------------------------------------------------------------Ran13testsin1.302s ona9bdce8: ~ ./pythonLib/test/test_tools/test_msgfmt.py ..............----------------------------------------------------------------------Ran14testsin1.305s |
Ah you are too fast 😆 |
c73d460
intopython:mainUh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedMay 1, 2025
|
Thanks@tomasr8 for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry,@tomasr8 and@serhiy-storchaka, I could not cleanly backport this to
|
GH-133255 is a backport of this pull request to the3.13 branch. |
Uh oh!
There was an error while loading.Please reload this page.
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 inpygettext
(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.