-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: Use standardized gql fragments in queries.js files #1147
Conversation
# Conflicts: # client/components/TestQueue2/queries.js
# Conflicts: # client/components/TestQueue/queries.js
…move as there's an issue with testing the loading element in 'e2e' test)
# Conflicts: # client/components/DataManagement/queries.js # client/components/TestQueue/queries.js # client/tests/__mocks__/GraphQLMocks/DataManagementPagePopulatedMock.js
# Conflicts: # client/tests/DataManagement.test.jsx
… enzyme dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressed you managed to change this much in place. I gave cross verification between the fragments and the old queries my best shot but obviously there are limits on what a human can accomplish there. As such, I agree that we are going to need additional testing to feel secure here.
For the purposes of this PR as it stands and how it will guide additional work before merging, I like where you drew the boundaries on the fragments. You've struck a good balance between reusability with a limited set of fragments and precision of fetch. Also all of the non-query updates make sense to me including the changes in the testing.
I will wait to mark this approved until we get the additional PRs in but for the purposes of continuing this work, I approve this. I'll reach out to you directly to coordinate writing additional tests.
# Conflicts: # client/tests/DataManagement.test.jsx # client/tests/__mocks__/GraphQLMocks/DataManagementPagePopulatedMock.js # client/tests/e2e/Reports.e2e.test.js # client/tests/e2e/TestPlanVersions.e2e.test.js # client/tests/e2e/TestReview.e2e.test.js # client/tests/e2e/TestRun.e2e.test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! As I said earlier, I like where the boundaries were drawn and a fragment for TestPlanVersion
can be a low priority follow-up.
I am amazed that it passes the new tests on the first try. Well done!
To supersede #876.
I've deviated from the original suggestion of:
Mainly because having significantly populated queries per page even if the performance hit was marginal (significant in some cases) became somewhat noisy.
So I did my best to identify common enough existing attributes being requested per page and included them in their own respective
client/components/common/fragments/TYPE.js
file. Where applicable, I created 2 separate queries for the graphql type (simple
andall
).Another oddity about this work is there is an inconsistency between the
testPlanVersionResolver
andtestPlanVersionsResolver
which makes using the...TestPlanVersion
fragment throw errors for some queries. It may not be worth resolving now due to the current size of the PR and could be follow up task if it becomes an issue.This PR is still missing several tests for components to feel confidence in this change, but I would rather not balloon this PR more. In discussing this with @stalgiag, they noted that the missing tests can be done in a follow up PR. So this shouldn't be merged until that work is done (so leaving as draft for now).