Skip to content
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

Closed
wants to merge 2 commits into from
Closed

DRY with lists tests #1178

wants to merge 2 commits into from

Conversation

mohawk2
Copy link
Contributor

@mohawk2 mohawk2 commented Dec 24, 2017

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.

Copy link
Contributor

@leebyron leebyron left a 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.

@leebyron
Copy link
Contributor

leebyron commented Jan 8, 2018

Also note that test coverage dropped as a result of this PR which is surprising for a test refactor.

@mohawk2
Copy link
Contributor Author

mohawk2 commented Feb 17, 2018

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants