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

Align BuildSchemaTest with graphql-js #928

Merged
merged 42 commits into from
Jun 9, 2022

Conversation

vhenzl
Copy link
Contributor

@vhenzl vhenzl commented Sep 2, 2021

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 with buildASTSchema-test.js (up to v15.5.2) and reveals where graphql-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 and graphql-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 to BuildSchemaLegacyTest. These are the corresponding commits in graphql-js:


Tests fail because:

v14.7 commit

v15.0 commit

  • extensionASTNodes is not set
  • missing @ in directive name in error messages
  • build doesn't throw on unknown types

v15.5 commit

  • different order of default directives

- 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
@spawnia
Copy link
Collaborator

spawnia commented Sep 2, 2021

The v14.0 commit could be merged (I can create a separated PR for it)

That would be great!

@spawnia
Copy link
Collaborator

spawnia commented May 29, 2022

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

@vhenzl
Copy link
Contributor Author

vhenzl commented Jun 1, 2022

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

@spawnia
Copy link
Collaborator

spawnia commented Jun 2, 2022

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.

@vhenzl
Copy link
Contributor Author

vhenzl commented Jun 2, 2022

I'm just working on it.

@vhenzl
Copy link
Contributor Author

vhenzl commented Jun 2, 2022

I have merged in master. To get it ready, I want to replace dedent() with <<<'GRAPHQL' and I need to review everything.

A few tests are failing:

testMatchOrderOfDefaultTypesAndDirectives - no idea what's happening there

testSupportsDescriptions - graphql/graphql-js@b883320

The remaining three (testSupportsDeprecated, testMaintainsIncludeSkipAndSpecifiedBy, and testAddingDirectivesMaintainsIncludeSkipAndSpecifiedBy) are related to #1140 and #110. But they can't be easily skiped/marked as incomplete as they test existing and missing functionality together (that's also true for schema description).

@spawnia How do you want to deal with the failing tests?

@vhenzl
Copy link
Contributor Author

vhenzl commented Jun 2, 2022

Just for the record, the tests were synced with v15.5. There are some differences from graphql-js v16. Shouldn't be anything important though. I'll see how much work it would be to include them here.

@spawnia
Copy link
Collaborator

spawnia commented Jun 2, 2022

The tests relating to missing features can be skipped, referencing the respective issues - I also created #1158.

testMatchOrderOfDefaultTypesAndDirectives - no idea what's happening there

We should adapt

$directives = array_map(
[$definitionBuilder, 'buildDirective'],
$directiveDefs
);
// If specified directives were not explicitly declared, add them.
$directivesByName = [];
foreach ($directives as $directive) {
$directivesByName[$directive->name][] = $directive;
}
if (! isset($directivesByName['skip'])) {
$directives[] = Directive::skipDirective();
}
if (! isset($directivesByName['include'])) {
$directives[] = Directive::includeDirective();
}
if (! isset($directivesByName['deprecated'])) {
$directives[] = Directive::deprecatedDirective();
}
, perhaps always keep the directives as a map?

@spawnia spawnia marked this pull request as ready for review June 8, 2022 10:11
@spawnia
Copy link
Collaborator

spawnia commented Jun 8, 2022

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

@vhenzl
Copy link
Contributor Author

vhenzl commented Jun 9, 2022

You can merge it.

@spawnia spawnia merged commit 0a92169 into webonyx:master Jun 9, 2022
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.

2 participants