-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: Remove shared mutable state between database instances #2777
fix: Remove shared mutable state between database instances #2777
Conversation
d6bb1ef
to
92aa500
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2777 +/- ##
===========================================
+ Coverage 78.19% 78.58% +0.40%
===========================================
Files 313 315 +2
Lines 23116 23810 +694
===========================================
+ Hits 18074 18711 +637
- Misses 3667 3719 +52
- Partials 1375 1380 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 21 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
7b55fb8
to
92aa500
Compare
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.
LGTM
Relevant issue(s)
Resolves #2774
Description
Removes shared mutable GQL state between database instances.
These global static GQL type variables are actually mutated by the GQL library we use at different points during database init, and schema update. This means that when multiple Defra instances are created in the same process they were sharing state. As well as just being an inherently bad thing, this also prevented us from running tests within packages in parallel using
t.Parallel()
as the race detector would complain.Our integration tests now can all run withThet.Parallel()
and the--race
flag (tested locally). I do not wish to make that change within this PR though for cleanliness/focus reasons.--race
flag is still flaky in the CI, this PR improves things but does not let us Parellize the tests fully. I believe that the shared state in https://github.com/graphql-go/graphql/blob/master/introspection.go is responsible for the remaining failures, I'd rather spend time moving away from the package entirely than to rework that, however I still think this PR represents an improvement and would like to merge.