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 test for variable coercion #1919

Merged
merged 3 commits into from
Jan 7, 2021
Merged

Add test for variable coercion #1919

merged 3 commits into from
Jan 7, 2021

Conversation

sungam3r
Copy link
Member

Small test to verify behavior for variable coercion and error for wrong literal.

@sungam3r sungam3r added the test Pull request that adds new or changes existing tests label Oct 15, 2020
@sungam3r sungam3r added this to the 3.1 milestone Oct 15, 2020
@sungam3r sungam3r requested a review from Shane32 October 15, 2020 08:59
@sungam3r
Copy link
Member Author

At first glance, it seems strange why one request works, and the second does not. This question came up on one of our development teams.

@sungam3r
Copy link
Member Author

Well, in fact, everything is a little more interesting. See graphql/graphql-js#1324
@Shane32 I remember you wondered at some places in code when serializing / passing scalars. It seems this is exactly the case. Good candidate for 3.2 milestone.

@sungam3r
Copy link
Member Author

If you want, then I can merge this test for 3.1 and then in 3.2 we will "fix" it. We can also defer this PR to 3.2 and make corrections right here.

@Shane32
Copy link
Member

Shane32 commented Oct 15, 2020

If you want, then I can merge this test for 3.1 and then in 3.2 we will "fix" it. We can also defer this PR to 3.2 and make corrections right here.

I’d probably at least add a todo note that this is unintended or incorrect behavior.

The way I’m planning to fix this will probably be breaking and need to be in 4.0. I can explain later. Maybe a patch could be implemented for 3.2. Good to have this test though.

@sungam3r sungam3r removed this from the 3.1 milestone Oct 15, 2020
@sungam3r sungam3r added this to the 3.2 milestone Oct 31, 2020
@Shane32 Shane32 modified the milestones: 3.2, 3.3 Dec 17, 2020
@Shane32
Copy link
Member

Shane32 commented Jan 7, 2021

@sungam3r I suggest we merge this test. It may be useful when I start reworking the input code.

@sungam3r sungam3r merged commit 07cd465 into master Jan 7, 2021
@sungam3r sungam3r deleted the coercion-test branch January 7, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants