search

graph-gophers / graphql-go

GraphQL server with a focus on ease of use
BSD 2-Clause "Simplified" License
4.64k stars 491 forks source link

Upgrade graphql-js Testdata Process #633

Closed dackroyd closed 5 months ago

dackroyd commented 5 months ago

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.

pavelnikolov commented 5 months ago

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 commented 5 months ago

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)