DRY with lists tests#1178
Conversation
leebyron
left a comment
There was a problem hiding this comment.
DRY isn't always what you want with tests - the more important factor is legibility so its easy to understand what is under test, what the expected values are, and how you might write similar code to exercise the same behavior in the source code.
I find this much harder to read - I think this either needs quite a bit more comments to explain what is under test or needs to be refactored.
As an example, reading a line like:
it('Contains values', check(type, [1, 2], expected.containsValues));I have no idea what is being tested, how [1, 2] relates to a tested or expected value, or what expected.containsValues are.
Similarly:
allChecks(GraphQLList(GraphQLInt), {
containsValues: { data: dataOk },
containsNull: { data: dataOkWithNull },
...
})I'm not sure what this is testing, or what the expected value is.
|
Also note that test coverage dropped as a result of this PR which is surprising for a test refactor. |
|
@leebyron I'm a bit surprised about the test coverage point. I assume it's because it's measuring the total code volume including tests, and if the tests get smaller, the amount of unexercised code will get proportionally larger? I have added comments hoping this renders the changes more legible. What do you think? |
With these changes, the "Execute: Handles list nullability" tests read almost like a DSL. I claim this makes the tests more understandable, and therefore more maintainable.