-
-
Notifications
You must be signed in to change notification settings - Fork 560
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
Align BuildSchemaTest
with graphql-js
#928
Conversation
- adds `dedent` where appropriate - adds indentation to GQL strings - fixes GQL in it('Simple Type') & it('Type modifiers') - updates it('Correctly assign AST nodes') - fixes expected and actual order in asserts - fixes assertGreaterThan to assertNotEmpty - preserves additional tests not in `graphql-js` v14.0
- adds missing tests, reflects changes to the tests - moves removed tests to BuildSchemaLegacyTest
That would be great! |
@vhenzl do you have an idea when you would pick this up again? I went through the open issues and pull requests, as far as I can tell this is the last one that should be in v15. |
@spawnia Not sure, don't have much time for it recently. From those big things, apart from the obvious specifiedBy directive, I believe support for input & args deprecation is still missing. Also, the schema builder still doesn't support schema extension, but that may not actually be needed for the tests to pass. |
Thanks for your feedback. I might pick this merge request up and finish it, I will tell you when I start working on it. The missing features seem like something that can be added without breaking changes, so we do not necessarily need to wait with v15. |
I'm just working on it. |
I have merged in master. To get it ready, I want to replace A few tests are failing:
The remaining three ( @spawnia How do you want to deal with the failing tests? |
Just for the record, the tests were synced with v15.5. There are some differences from |
The tests relating to missing features can be skipped, referencing the respective issues - I also created #1158.
We should adapt graphql-php/src/Utils/BuildSchema.php Lines 177 to 198 in 98a10e6
|
@vhenzl I made the necessary changes to make the tests pass. Let me know if you would like to make other changes or I should merge this as is. We can fix the partial/skipped tests when implementing the missing features. |
You can merge it. |
While working on tests for #921, I noticed that tests for schema building and extending are not in pair with the reference implementation.
This PR aligns
BuildSchemaTest.php
withbuildASTSchema-test.js
(up to v15.5.2) and reveals wheregraphql-php
lags behind.The first commit (v14.0) is the messiest one (a lot of various changes), but the only one where tests are passing. From the v14.7 commit up, the behaviour between
graphql-php
andgraphql-js
starts to differ and tests to fail.The v14.0 commit could be merged (I can create a separated PR for it), the other commits are currently more for reference only and can be cherry-picked when a related problem is resolved.
I'm planning to do the same for
SchemaExtenderTest
.Tests no longer present in
buildASTSchema-test.js
have been moved toBuildSchemaLegacyTest
. These are the corresponding commits ingraphql-js
:Tests fail because:
v14.7 commit
type Query
orunion EmptyUnion
) (fixed in Fix printing of empty types #940)v15.0 commit
extensionASTNodes
is not set@
in directive name in error messagesv15.5 commit