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
ecaa9d1 to
c4fb7ac
Compare
StanFromIreland
left a comment
There was a problem hiding this comment.
We can inherit from GettextBaseTest and avail of the mo files. You can then use the messages in the files to test it works properly.
43ffa67 to
77ffbe6
Compare
StanFromIreland
left a comment
There was a problem hiding this comment.
We can still simply them a lot, you have to remember dgettext is really just a wrapper.
|
|
||
|
|
||
| # 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.
Lib/test/test_gettext.py
Outdated
| 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
Lib/test/test_gettext.py
Outdated
| 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.
Lib/test/test_gettext.py
Outdated
| 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).
|
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) |
Lib/test/test_gettext.py
Outdated
| 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?
There was a problem hiding this comment.
Seems reset_gettext() resets the state from GettextBaseTest is enough
8824d95 to
c57f95b
Compare
c57f95b to
ae5840f
Compare
add tests for dgettext
dgettext#134593gettext#130655