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

add testcases for nested list coercion #1528

Merged
merged 3 commits into from
Sep 27, 2018
Merged

add testcases for nested list coercion #1528

merged 3 commits into from
Sep 27, 2018

Conversation

jjergus
Copy link
Contributor

@jjergus jjergus commented Sep 20, 2018

Add some test cases to cover coercion of list values, especially nested lists where some non-list values are involved. These exercise the following special case from the spec:

If the value passed as an input to a list type is not a list and not the null value, then the result of input coercion is a list of size one, where the single item value is the result of input coercion for the list’s item type on the provided value (note this may apply recursively for nested lists).

Most of these examples are copied directly from the spec. However, one of them actually coerces to a value different from what the example in the spec says, which I believe to be a mistake in the spec: graphql/graphql-spec#515

@IvanGoncharov what do you think?

@@ -283,4 +284,39 @@ describe('coerceValue', () => {
]);
});
});

describe('for nested GraphQLList', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjergus Thanks for PR 👍
I think it better to have describe for for GraphQLList and just implement all test from this table: http://facebook.github.io/graphql/June2018/#sec-Type-System.List

describe('for nested GraphQLList', () => {
const TestList = GraphQLList(GraphQLList(GraphQLInt));

it('correctly coerces a nested list value', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use existing naming convention, e.g.

it('returns value for integer', () => {
const result = coerceValue(1, GraphQLFloat);
expectValue(result).to.equal(1);
});

it('returns null for null value', () => {
const result = coerceValue(null, GraphQLInt);
expectValue(result).to.equal(null);
});

So instead of correctly coerces a nested list value it could be return value for nested list.
Same goes for other testcases

@jjergus
Copy link
Contributor Author

jjergus commented Sep 20, 2018

Thanks for the review! I added more test cases and made the descriptions consistent with the rest of the file. I'm still keeping the nested list test cases separate from the non-nested ones, since it seems to be easier to read that way.

Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good set of tests. Thanks!

@jjergus jjergus merged commit dec24f9 into graphql:master Sep 27, 2018
@jjergus jjergus deleted the list-test2 branch September 27, 2018 15:54
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.

4 participants