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

chore: warehouse tests race detection #3773

Merged
merged 16 commits into from
Aug 28, 2023
Merged

chore: warehouse tests race detection #3773

merged 16 commits into from
Aug 28, 2023

Conversation

fracasula
Copy link
Collaborator

@fracasula fracasula commented Aug 22, 2023

Description

Running warehouse tests with -race.

Changelog

  • running warehouse tests with -race in order to be able to detect bugs due unregulated concurrent access to the same variable(s)
  • added a way to clone model.Schema because even if we use a mutex to protect the concurrent access, we're still passing around a reference so a change to the map would still count as a data race
  • moved Schema into its own package that now has both a public and a private API
    • this was done to decouple and to make sure all concurrent accesses are handled properly since the mutexes are now encapsulated in the type
    • moved public API (exported methods) on top
  • moved uploadSchema into UploadJob
  • moved handleSchemaChange and its errors to slave_worker since it's specific to that business logic

Linear Ticket

< Linear Link >

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@linear
Copy link

linear bot commented Aug 22, 2023

PIPE-119 Run Warehouse tests with -race

We should change our continous integration pipeline to run the warehouse tests with -race.

Any data race detected is going to make the tests fail even if all assertions pass so resulting races should be eliminated as well.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 93.65% and project coverage change: +0.03% 🎉

Comparison is base (890ccb6) 68.98% compared to head (869be23) 69.02%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3773      +/-   ##
==========================================
+ Coverage   68.98%   69.02%   +0.03%     
==========================================
  Files         348      349       +1     
  Lines       51834    51886      +52     
==========================================
+ Hits        35760    35815      +55     
+ Misses      13772    13770       -2     
+ Partials     2302     2301       -1     
Files Changed Coverage Δ
warehouse/router_identities.go 19.83% <0.00%> (ø)
warehouse/schema/schema.go 96.00% <92.59%> (ø)
warehouse/upload.go 71.90% <92.98%> (+0.38%) ⬆️
warehouse/internal/model/schema.go 100.00% <100.00%> (ø)
warehouse/slave_worker.go 83.50% <100.00%> (+1.86%) ⬆️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -293,8 +293,6 @@ func TestIntegration(t *testing.T) {
tc := tc

t.Run(tc.name, func(t *testing.T) {
t.Parallel()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will go away anyway due to this PR.

@fracasula fracasula merged commit ce2ed33 into master Aug 28, 2023
35 checks passed
@fracasula fracasula deleted the chore.pipe-119 branch August 28, 2023 08:42
achettyiitr added a commit that referenced this pull request Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants