-
Notifications
You must be signed in to change notification settings - Fork 2k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRY with lists tests #1178
DRY with lists tests #1178
Conversation
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.
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.