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

Test for data race conditions in adapters #1756

Merged
merged 20 commits into from
May 12, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions adapters/adapterstest/test_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte)
}
}

// testMakeBidsImpl asserts the results of the bidder MakeRequests implementation against the
// testMakeRequestsImpl asserts the results of the bidder MakeRequests implementation against the
// expected JSON-defined results and makes sure the adapter's implementations of MakeRequests do
// not incurr in data races
func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData {
Expand All @@ -343,9 +343,9 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder
// compared inside assertNoDataRace() to verify no data races occur.
// - The shallow copy is helpful because it provides reference values to shared memory that we don't want
// adapters to modify.
// - The deep copy will help us preserve all the original values, even those of the shared memory fields, that
// will remain untouched by the adapter tests so we can compare the real shared memory fields (that can
// be accessed via the shallow copy) to their original values
// - The deep copy will help us preserve all the original values, even those of the shared memory fields that
// will remain untouched by the adapter tests so we can compare the real shared memory (that can
// be accessed via the shallow copy) to its original values
func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest) {
deepReqCopy := deepcopy.Copy(original).(*openrtb2.BidRequest)

Expand Down Expand Up @@ -375,6 +375,16 @@ func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRe
assert.Equal(t, bidRequestBefore.Source, bidRequestAfter.Source, "Data race in BidRequest.Source field in file %s", filename)
assert.Equal(t, bidRequestBefore.Regs, bidRequestAfter.Regs, "Data race in BidRequest.Regs field in file %s", filename)

// Assert slice fields were not modified by bidder adapter MakeRequests implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add in this comment that the Imp[] slice is purposefully excluded.

assert.Equal(t, bidRequestBefore.WSeat, bidRequestAfter.WSeat, "Data race in BidRequest.[]WSeat array")
assert.Equal(t, bidRequestBefore.BSeat, bidRequestAfter.BSeat, "Data race in BidRequest.[]BSeat array")
assert.Equal(t, bidRequestBefore.Cur, bidRequestAfter.Cur, "Data race in BidRequest.[]Cur array")
assert.Equal(t, bidRequestBefore.WLang, bidRequestAfter.WLang, "Data race in BidRequest.[]WLang array")
assert.Equal(t, bidRequestBefore.BCat, bidRequestAfter.BCat, "Data race in BidRequest.[]BCat array")
assert.Equal(t, bidRequestBefore.BAdv, bidRequestAfter.BAdv, "Data race in BidRequest.[]BAdv array")
assert.Equal(t, bidRequestBefore.BApp, bidRequestAfter.BApp, "Data race in BidRequest.[]BApp array")
assert.Equal(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a string, not a slice. It's ok for the bidder to modify it since strings in Go are immutable.


// Assert Imps separately
assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename)
}
Expand All @@ -393,7 +403,8 @@ func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb2.Imp, impsAfter []o
assert.Equal(t, impsBefore[i].Native, impsAfter[i].Native, "Data race in bidRequest.Imp[%d].Native field. File:%s", i, filename)
assert.Equal(t, impsBefore[i].PMP, impsAfter[i].PMP, "Data race in bidRequest.Imp[%d].PMP field. File:%s", i, filename)
assert.Equal(t, impsBefore[i].Secure, impsAfter[i].Secure, "Data race in bidRequest.Imp[%d].Secure field. File:%s", i, filename)
assert.ElementsMatch(t, impsBefore[i].Metric, impsAfter[i].Metric, "Data race in bidRequest.Imp[%d].[]Metric array. File:%s", i)
assert.Equal(t, impsBefore[i].Metric, impsAfter[i].Metric, "Data race in bidRequest.Imp[%d].[]Metric array. File:%s", i)
assert.Equal(t, impsBefore[i].IframeBuster, impsAfter[i].IframeBuster, "Data race in bidRequest.Imp[%d].[]IframeBuster array", i)
}
}
}
Expand Down