Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-130655: add tests for dgettext#134594
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
base:main
Are you sure you want to change the base?
Conversation
Lib/test/test_gettext.py Outdated
| return mofile | ||
| def test_dgettext_found_translation(self): | ||
| """Test dgettext finds translation in specified domain.""" |
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.
Avoid using docstrings fortest_* methods as it will be shown upon failure.
Lib/test/test_gettext.py Outdated
| def setUp(self): | ||
| """Set up a specific test domain and environment for dgettext tests.""" | ||
| self.localedir = tempfile.mkdtemp() |
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.
Instead of usingmkdtemp which would create something inside/tmp, usetest.support.temp_dir (I think, check the name) which creates something inside the build folder (it's easier to cleanup)
Lib/test/test_gettext.py Outdated
| def setUp(self): | ||
| """Set up a specific test domain and environment for dgettext tests.""" | ||
| self.localedir = tempfile.mkdtemp() | ||
| self.addCleanup(shutil.rmtree, self.localedir) |
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.
Prefer usingos_helper.rmtree overshutil.rmtree
ecaa9d1 toc4fb7acCompareThere 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.
We can inherit fromGettextBaseTest and avail of the mo files. You can then use the messages in the files to test it works properly.
Uh oh!
There was an error while loading.Please reload this page.
43ffa67 to77ffbe6CompareThere 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.
We can still simply them a lot, you have to remember dgettext is really just a wrapper.
| @@ -11,7 +11,6 @@ | |||
| # TODO: | |||
| # - Add new tests, for example for "dgettext" | |||
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.
Let's keep the TODO, since it is still valid (the first part).
It can be updated with the issue number if you wish.
| def test_dgettext_luxury_yacht_translation(self): | ||
| result = gettext.dgettext('gettext', 'Raymond Luxury Yach-t') | ||
| self.assertEqual(result, 'Throatwobbler Mangrove') | ||
| def test_dgettext_nudge_nudge_translation(self): | ||
| result = gettext.dgettext('gettext', 'nudge nudge') | ||
| self.assertEqual(result, 'wink wink') | ||
| def test_dgettext_multiline_translation(self): |
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 don't think we need these these anyway, it's long and pretty much identical to the previous two. dgettext is a pretty simple wrapper. Maybe merge the first twotest_dgettext_translation for organization
| if domain == '': | ||
| expected = gettext.gettext(message) | ||
| else: | ||
| expected = message |
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.
Why not just store the expected, it is clearer. And this part is made simpler.
| GettextBaseTest.setUp(self) | ||
| gettext.bindtextdomain('gettext', os.curdir) | ||
| def test_dgettext_found_translation(self): |
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.
Join this one with the other two, these all test the exact same thing (which is well tested above too).
Thanks for the PR@alex-semenyuk! Would you mindmeasuring the test coverage before and after this change and sharing the results here? (after you address the review comments) |
Uh oh!
There was an error while loading.Please reload this page.
add tests for dgettext
dgettext#134593gettext#130655