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

Upgrade graphql-js Testdata Process #633

Merged

Conversation

dackroyd
Copy link
Contributor

@dackroyd dackroyd commented Mar 25, 2024

The process for pulling in test cases from graphql-js has been broken for a long time. The project has made an array of changes which prevent the existing approach from being functional, including migration to Typescript, changes to assertion helper functions and more.

With this change, the approach for integration is shifted. Rather than needing to clone the graphql-js repo, instead we pull it in as a dependency of the Node.js project under testdata. This is patched via patch-package in a postinstall hook, which causes the test functions to write the cases out, and those are collected and written to JSON.

As expected, the test cases have a significant number of updates, and the Go tests driven by them fail in their current state, so further work is required.

Parity with test files pulled in from graphql-js prior to this change is maintained. A number of test files were excluded due to failures, and others have been introduced since: these are left alone for now, as it is expected that enabling those will require larger changes to the implementation to satisfy them.

Tests that have been added manually to the tests.json file have also been abandoned here. The JSON file is generated entirely by the process defined, and additional test cases will need to be recovered and re-implemented in other ways if they are still valuable.

Ensure consistent behaviour with graphql-js, matching expected test outputs. In many cases this has been a change to quoting and formatting of identifiers, however other changes to application of rules have also been applied to satisfy they required behaviours.

Rule names exercised by these tests are updated to match what has changed in graphql-js. Other rules however have been left as-is, with none of the tests from upstream validating those. As additional tests are enabled and behaviour brought inline with that, these should be updated at that time.

@dackroyd dackroyd force-pushed the update-graphql-js-testdata-process branch from b1d35b1 to 8e0039d Compare April 1, 2024 08:02
@dackroyd dackroyd force-pushed the update-graphql-js-testdata-process branch from 8e0039d to a41f157 Compare April 2, 2024 11:04
@dackroyd dackroyd marked this pull request as ready for review April 2, 2024 11:05
The process for pulling in test cases from graphql-js has been broken
for a long time. The project has made an array of changes which prevent
the existing approach from being functional, including migration to
Typescript, changes to assertion helper functions and more.

With this change, the approach for integration is shifted. Rather than
needing to clone the graphql-js repo, instead we pull it in as a
dependency of the Node.js project under testdata. This is patched via
patch-package in a postinstall hook, which causes the test functions to
write the cases out, and those are collected and written to JSON.

As expected, the test cases have a significant number of updates, and
the Go tests driven by them fail in their current state, so further work
is required.

Parity with test files pulled in from graphql-js prior to this change is
maintained. A number of test files were excluded due to failures, and
others have been introduced since: these are left alone for now, as it
is expected that enabling those will require larger changes to the
implementation to satisfy them.

Tests that have been added manually to the tests.json file have also been
abandoned here. The JSON file is generated entirely by the process defined,
and additional test cases will need to be recovered and re-implemented in
other ways if they are still valuable.
Ensure consistent behaviour with graphql-js, matching expected
test outputs. In many cases this has been a change to quoting and
formatting of identifiers, however other changes to application of
rules have also been applied to satisfy they required behaviours.

Rule names exercised by these tests are updated to match what has
changed in graphql-js. Other rules however have been left as-is, with
none of the tests from upstream validating those. As additional tests
are enabled and behaviour brought inline with that, these should be
updated at that time.
@dackroyd dackroyd force-pushed the update-graphql-js-testdata-process branch from a41f157 to ad43f96 Compare April 2, 2024 11:07
@pavelnikolov
Copy link
Member

LGTM. I believe that this is a good start and improvements and new test cases can be added incrementally. Now at least we have some automated validation test coverage.
Do you believe that there is something important left to do for this PR?

@dackroyd dackroyd changed the title WIP: Upgrade graphql-js Testdata Process Upgrade graphql-js Testdata Process Apr 2, 2024
@dackroyd
Copy link
Contributor Author

dackroyd commented Apr 2, 2024

In its current state, I believe this change is ready to go. Updates pushed earlier tonight should address the outstanding issues, and ensure all tests run successfully (or are skipped)

@pavelnikolov pavelnikolov merged commit a3d0ced into graph-gophers:master Apr 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants