gh-130655: add tests for dgettext #134594
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.
Avoid using docstrings for test_* 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.
Instead of using mkdtemp which would create something inside /tmp, use test.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.
Prefer using os_helper.rmtree over shutil.rmtree
alex-semenyuk
force-pushed
the
add_dgettext_tests
branch
from
ecaa9d1 to
c4fb7ac
Compare
picnixz
changed the title
picnixz
added
needs backport to 3.13
alex-semenyuk
force-pushed
the
add_dgettext_tests
branch
from
43ffa67 to
77ffbe6
Compare
| @@ -11,7 +11,6 @@ | |||
|
|
|||
|
|
|||
| # TODO: | |||
| # - Add new tests, for example for "dgettext" | |||
There was a problem hiding this comment.
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.
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 two test_dgettext_translation for organization
| if domain == '': | ||
| expected = gettext.gettext(message) | ||
| else: | ||
| expected = message |
There was a problem hiding this comment.
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.
Join this one with the other two, these all test the exact same thing (which is well tested above too).
picnixz
requested review from
tomasr8 and
picnixz
and removed request for
picnixz
|
Thanks for the PR @alex-semenyuk! Would you mind measuring the test coverage before and after this change and sharing the results here? (after you address the review comments) |
| class DGettextTest(GettextBaseTest): | ||
|
|
||
| def setUp(self): | ||
| GettextBaseTest.setUp(self) |
There was a problem hiding this comment.
| GettextBaseTest.setUp(self) | |
| super().setUp() |
|
|
||
| def setUp(self): | ||
| GettextBaseTest.setUp(self) | ||
| gettext.bindtextdomain('gettext', os.curdir) |
There was a problem hiding this comment.
Don't we need to tearDown this one?
add tests for dgettext
dgettext#134593gettext#130655