checkPot: Add unit tests#11179
Conversation
feerrenrut
left a comment
There was a problem hiding this comment.
It's not clear to me if there is a test here that checks for success specifically. IE. not an error, not an unexpected success, and not an expected error. I can see that there could be, but it might be helpful if this was "called out" explicitly.
| def test_checkPot_lastMessage(self): | ||
| """Test that missing translators comment on the last message are reported.""" | ||
| self.assertEqual( | ||
| self.doCheckPot("firstMessage.pot"), |
There was a problem hiding this comment.
I think these tests would be easier to read if the test data was inline, preferably using a template so that just the test specific data is visible. On the other hand it's probably a good thing that this tests it end to end.
There was a problem hiding this comment.
Not easy to do without refactoring checkPot itself, as it currently only takes a filename parameter.
Furthermore, the test data currently sums up to 208 lines, which IMHO would reduce a whole lot the readability of the 98 lines long code.
Please note that the header of each data file also describes what errors are expected to be found in it.
EDIT: Sorry I over-read your comment. Templating would surely reduce the data footprint. Still checkPot only takes a filename, and I guess we are in no urge to refactor this.
There was a problem hiding this comment.
I agree for now this is fine. Mostly I'm thinking about the ease of reading the tests, going back and forth to data files to understand the cases being tested can be a pain. One benefit of succinct unit tests is the way they document code to developers, and invite developers to add variations to understand how the code works under different circumstances.
|
@feerrenrut wrote:
Added an "allOk" test case: a38bb20 |
feerrenrut
left a comment
There was a problem hiding this comment.
Thanks @JulienCochuyt
Link to issue number:
Follow-up of PR #11093
Summary of the issue:
PR #10941 brought to light flaws in
checkPotregarding handling of long messages - fixed by PR #11093@feerrenrut requested in #11093 (comment) the creation of unit tests to help limit occurrence of future regressions.
Description of how this pull request fixes the issue:
Added 6 unit tests for
checkPot, namely:Testing performed:
Well, for one thing... ran the unit tests!
More seriously, checked the test did fail when altering their input data and their expected output.
Known issues with pull request:
Change log entry:
I don't think this deserves a change log entry.