-
Notifications
You must be signed in to change notification settings - Fork 491
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
Upgrade graphql-js Testdata Process #633
Conversation
b1d35b1
to
8e0039d
Compare
8e0039d
to
a41f157
Compare
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.
a41f157
to
ad43f96
Compare
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. |
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) |
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.