From 7074f58e7dc5a698d00e3da34806532ddfea429b Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Wed, 17 Mar 2021 02:41:30 -0400 Subject: [PATCH 01/15] Data race test first draft --- adapters/adapterstest/test_json.go | 117 ++++++++++++++++++++++++++--- go.mod | 1 + go.sum | 2 + 3 files changed, 109 insertions(+), 11 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index a25a4f1905a..36eae81356c 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -7,8 +7,10 @@ import ( "regexp" "testing" + "github.com/jinzhu/copier" "github.com/mxmCherry/openrtb" "github.com/prebid/prebid-server/adapters" + "github.com/stretchr/testify/assert" "github.com/yudai/gojsondiff" "github.com/yudai/gojsondiff/formatter" @@ -107,23 +109,67 @@ func runSpec(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidd } else if isVideoTest { reqInfo.PbsEntryPoint = "video" } - actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, &reqInfo) - diffErrorLists(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors) - diffHttpRequestLists(t, filename, actualReqs, spec.HttpCalls) - bidResponses := make([]*adapters.BidderResponse, 0) + actualReqs := testMakeRequestsImpl(t, filename, spec, bidder, &reqInfo) + + testMakeBidsImpl(t, filename, spec, bidder, actualReqs) +} + +func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { + t.Helper() + + // Save original bidRequest values to assert no data races occur inside MakeRequests latter + deepBidReqCopy := openrtb.BidRequest{} + copier.Copy(&deepBidReqCopy, &spec.BidRequest) + shallowBidReqCopy := spec.BidRequest + + // Save original []Imp elements to assert no data races occur inside MakeRequests latter + deepImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp)) + shallowImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp)) + for i := 0; i < len(spec.BidRequest.Imp); i++ { + deepImpCopy := openrtb.Imp{} + copier.Copy(&deepImpCopy, &spec.BidRequest.Imp[i]) + deepImpCopies = append(deepImpCopies, deepImpCopy) + + shallowImpCopy := spec.BidRequest.Imp[i] + shallowImpCopies = append(shallowImpCopies, shallowImpCopy) + } + + // Run MakeRequests + actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) + + // Compare MakeRequests actual output versus expected values found in JSON file + assertErrorList(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors) + assertMakeRequestsOutput(t, filename, actualReqs, spec.HttpCalls) + + // Assert no data races occur using original bidRequest copies of references and values + assertNoDataRace(t, &deepBidReqCopy, &shallowBidReqCopy, deepImpCopies, shallowImpCopies) + + return actualReqs +} + +func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) { + t.Helper() + bidResponses := make([]*adapters.BidderResponse, 0) var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors)) - for i := 0; i < len(actualReqs); i++ { + + // We should have as many bids as number of adapters.RequestData found in MakeRequests output + for i := 0; i < len(makeRequestsOut); i++ { + // Run MakeBids with JSON refined spec.HttpCalls info that was asserted to match MakeRequests + // output inside testMakeRequestsImpl thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t)) + bidsErrs = append(bidsErrs, theseErrs...) bidResponses = append(bidResponses, thisBidResponse) } - diffErrorLists(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors) + // Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors + assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors) + // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids for i := 0; i < len(spec.BidResponses); i++ { - diffBidLists(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids) + assertMakeBidsOutput(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids) } } @@ -194,8 +240,8 @@ type expectedBid struct { // // Marshalling the structs and then using a JSON-diff library isn't great either, since -// diffHttpRequests compares the actual http requests to the expected ones. -func diffHttpRequestLists(t *testing.T, filename string, actual []*adapters.RequestData, expected []httpCall) { +// assertMakeRequestsOutput compares the actual http requests to the expected ones. +func assertMakeRequestsOutput(t *testing.T, filename string, actual []*adapters.RequestData, expected []httpCall) { t.Helper() if len(expected) != len(actual) { @@ -206,7 +252,7 @@ func diffHttpRequestLists(t *testing.T, filename string, actual []*adapters.Requ } } -func diffErrorLists(t *testing.T, description string, actual []error, expected []testSpecExpectedError) { +func assertErrorList(t *testing.T, description string, actual []error, expected []testSpecExpectedError) { t.Helper() if len(expected) != len(actual) { @@ -227,7 +273,7 @@ func diffErrorLists(t *testing.T, description string, actual []error, expected [ } } -func diffBidLists(t *testing.T, filename string, actual []*adapters.TypedBid, expected []expectedBid) { +func assertMakeBidsOutput(t *testing.T, filename string, actual []*adapters.TypedBid, expected []expectedBid) { t.Helper() if len(actual) != len(expected) { @@ -316,3 +362,52 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) } } } + +// assertNoDataRace compares the contents of the reference fields found in the original openrtb.BidRequest to their +// original values to make sure they were not modified and we are not incurring indata races. In order to assert +// no data races occur in the []Imp array, we call assertNoImpsDataRace() +func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidRequestAfter *openrtb.BidRequest, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) { + t.Helper() + + // Assert reference fields were not modified by bidder adapter MakeRequests implementation + assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field") + assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field") + assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field") + assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field") + assert.Equal(t, bidRequestBefore.Source, bidRequestAfter.Source, "Data race in BidRequest.Source field") + assert.Equal(t, bidRequestBefore.Regs, bidRequestAfter.Regs, "Data race in BidRequest.Regs field") + + // Assert slice fields were not modified by bidder adapter MakeRequests implementation + assert.ElementsMatch(t, bidRequestBefore.WSeat, bidRequestAfter.WSeat, "Data race in BidRequest.[]WSeat array") + assert.ElementsMatch(t, bidRequestBefore.BSeat, bidRequestAfter.BSeat, "Data race in BidRequest.[]BSeat array") + assert.ElementsMatch(t, bidRequestBefore.Cur, bidRequestAfter.Cur, "Data race in BidRequest.[]Cur array") + assert.ElementsMatch(t, bidRequestBefore.WLang, bidRequestAfter.WLang, "Data race in BidRequest.[]WLang array") + assert.ElementsMatch(t, bidRequestBefore.BCat, bidRequestAfter.BCat, "Data race in BidRequest.[]BCat array") + assert.ElementsMatch(t, bidRequestBefore.BAdv, bidRequestAfter.BAdv, "Data race in BidRequest.[]BAdv array") + assert.ElementsMatch(t, bidRequestBefore.BApp, bidRequestAfter.BApp, "Data race in BidRequest.[]BApp array") + assert.ElementsMatch(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array") + + // Assert Imps separately + assertNoImpsDataRace(t, impsBefore, impsAfter) +} + +// assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to +// their original values to make sure they were not modified and we are not incurring in data races. +func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) { + t.Helper() + + assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called") + + // Assert no data races occured in individual Imp elements + for i := 0; i < len(impsBefore); i++ { + assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field", i) + assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field", i) + assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field", i) + assert.Equal(t, impsBefore[i].Native, impsAfter[i].Native, "Data race in bidRequest.Imp[%d].Native field", i) + assert.Equal(t, impsBefore[i].PMP, impsAfter[i].PMP, "Data race in bidRequest.Imp[%d].PMP field", i) + assert.Equal(t, impsBefore[i].Secure, impsAfter[i].Secure, "Data race in bidRequest.Imp[%d].Secure field", i) + + assert.ElementsMatch(t, impsBefore[i].Metric, impsAfter[i].Metric, "Data race in bidRequest.Imp[%d].[]Metric array", i) + assert.ElementsMatch(t, impsBefore[i].IframeBuster, impsAfter[i].IframeBuster, "Data race in bidRequest.Imp[%d].[]IframeBuster array", i) + } +} diff --git a/go.mod b/go.mod index 48fc6b6479b..b2125269e4d 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b github.com/hashicorp/hcl v1.0.0 // indirect github.com/influxdata/influxdb v1.6.1 + github.com/jinzhu/copier v0.2.8 github.com/julienschmidt/httprouter v1.1.0 github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 // indirect github.com/kr/pretty v0.2.0 // indirect diff --git a/go.sum b/go.sum index 6da3f8898ba..10f4fbda8c8 100644 --- a/go.sum +++ b/go.sum @@ -44,6 +44,8 @@ github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= github.com/influxdata/influxdb v1.6.1 h1:OseoBlzI5ftNI/bczyxSWq6PKRCNEeiXvyWP/wS5fB0= github.com/influxdata/influxdb v1.6.1/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY= +github.com/jinzhu/copier v0.2.8 h1:N8MbL5niMwE3P4dOwurJixz5rMkKfujmMRFmAanSzWE= +github.com/jinzhu/copier v0.2.8/go.mod h1:24xnZezI2Yqac9J61UC6/dG/k76ttpq0DdJI3QmUvro= github.com/julienschmidt/httprouter v1.1.0 h1:7wLdtIiIpzOkC9u6sXOozpBauPdskj3ru4EI5MABq68= github.com/julienschmidt/httprouter v1.1.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 h1:uC1QfSlInpQF+M0ao65imhwqKnz3Q2z/d8PWZRMQvDM= From cbcc2937c0fd8a26d81fdccd3005d47d852bad8f Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 18 Mar 2021 10:46:07 -0400 Subject: [PATCH 02/15] Step out real quick --- adapters/adapterstest/test_json.go | 35 ++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 36eae81356c..138dc10a855 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -120,7 +120,8 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder // Save original bidRequest values to assert no data races occur inside MakeRequests latter deepBidReqCopy := openrtb.BidRequest{} - copier.Copy(&deepBidReqCopy, &spec.BidRequest) + copier.CopyWithOption(&deepBidReqCopy, &spec.BidRequest, copier.Option{DeepCopy: true}) + //copier.Copy(&deepBidReqCopy, &spec.BidRequest) shallowBidReqCopy := spec.BidRequest // Save original []Imp elements to assert no data races occur inside MakeRequests latter @@ -388,7 +389,8 @@ func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidReq assert.ElementsMatch(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array") // Assert Imps separately - assertNoImpsDataRace(t, impsBefore, impsAfter) + assertNoImpsDataRace(t, bidRequestBefore.Imp, impsAfter) + //assertNoImpsDataRace(t, impsBefore, impsAfter) } // assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to @@ -411,3 +413,32 @@ func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []op assert.ElementsMatch(t, impsBefore[i].IframeBuster, impsAfter[i].IframeBuster, "Data race in bidRequest.Imp[%d].[]IframeBuster array", i) } } + +func TestACopy() { + app := openrtb.App{ + ID: "anyID", + Name: "anyName", + Bundle: "anyBundle", + Domain: "anyDomain", + StoreURL: "anyUrl.com", + Cat: []string{"AAA", "BBB"}, + SectionCat: []string{"aaa", "bbb"}, + PageCat: []string{"CCC", "DDD"}, + Ver: "VER", + PrivacyPolicy: 1, + Paid: 2, + Publisher: &openrtb.Publisher{ + ID: "anyPublisherID", + Name: "anyPublisherName", + Cat: []string{"pubAAA", "pubBBB"}, + Domain: "anyPublisherDomain", + Ext: json.RawMessage(`{"appPublisherExtField":"value"}`), + }, + Content: &openrtb.Content{ + ID: "anyAppContentID", + Episode: 1, + }, + Keywords: "AnyKeyword", + Ext: json.RawMessage(`{"appExtField":"value"}`), + } +} From a4c6c4a1c753319390e3e426cc68ae6677f130d3 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 18 Mar 2021 11:02:48 -0400 Subject: [PATCH 03/15] Rollback CopyWithOption --- adapters/adapterstest/test_json.go | 64 +++++++++++++++--------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 138dc10a855..f583d502ecc 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -120,8 +120,8 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder // Save original bidRequest values to assert no data races occur inside MakeRequests latter deepBidReqCopy := openrtb.BidRequest{} - copier.CopyWithOption(&deepBidReqCopy, &spec.BidRequest, copier.Option{DeepCopy: true}) - //copier.Copy(&deepBidReqCopy, &spec.BidRequest) + //copier.CopyWithOption(&deepBidReqCopy, &spec.BidRequest, copier.Option{DeepCopy: true}) + copier.Copy(&deepBidReqCopy, &spec.BidRequest) shallowBidReqCopy := spec.BidRequest // Save original []Imp elements to assert no data races occur inside MakeRequests latter @@ -389,8 +389,8 @@ func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidReq assert.ElementsMatch(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array") // Assert Imps separately - assertNoImpsDataRace(t, bidRequestBefore.Imp, impsAfter) - //assertNoImpsDataRace(t, impsBefore, impsAfter) + //assertNoImpsDataRace(t, bidRequestBefore.Imp, impsAfter) + assertNoImpsDataRace(t, impsBefore, impsAfter) } // assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to @@ -414,31 +414,31 @@ func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []op } } -func TestACopy() { - app := openrtb.App{ - ID: "anyID", - Name: "anyName", - Bundle: "anyBundle", - Domain: "anyDomain", - StoreURL: "anyUrl.com", - Cat: []string{"AAA", "BBB"}, - SectionCat: []string{"aaa", "bbb"}, - PageCat: []string{"CCC", "DDD"}, - Ver: "VER", - PrivacyPolicy: 1, - Paid: 2, - Publisher: &openrtb.Publisher{ - ID: "anyPublisherID", - Name: "anyPublisherName", - Cat: []string{"pubAAA", "pubBBB"}, - Domain: "anyPublisherDomain", - Ext: json.RawMessage(`{"appPublisherExtField":"value"}`), - }, - Content: &openrtb.Content{ - ID: "anyAppContentID", - Episode: 1, - }, - Keywords: "AnyKeyword", - Ext: json.RawMessage(`{"appExtField":"value"}`), - } -} +// func TestACopy() { +// app := openrtb.App{ +// ID: "anyID", +// Name: "anyName", +// Bundle: "anyBundle", +// Domain: "anyDomain", +// StoreURL: "anyUrl.com", +// Cat: []string{"AAA", "BBB"}, +// SectionCat: []string{"aaa", "bbb"}, +// PageCat: []string{"CCC", "DDD"}, +// Ver: "VER", +// PrivacyPolicy: 1, +// Paid: 2, +// Publisher: &openrtb.Publisher{ +// ID: "anyPublisherID", +// Name: "anyPublisherName", +// Cat: []string{"pubAAA", "pubBBB"}, +// Domain: "anyPublisherDomain", +// Ext: json.RawMessage(`{"appPublisherExtField":"value"}`), +// }, +// Content: &openrtb.Content{ +// ID: "anyAppContentID", +// Episode: 1, +// }, +// Keywords: "AnyKeyword", +// Ext: json.RawMessage(`{"appExtField":"value"}`), +// } +// } From dde3442b6c7d8caecca052952862429db05e14de Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Wed, 24 Mar 2021 21:22:08 -0400 Subject: [PATCH 04/15] Implemented bid request deep copy function instead of importing library --- adapters/adapterstest/test_json.go | 411 ++++++++++++++++++++--------- go.mod | 1 - go.sum | 2 - 3 files changed, 291 insertions(+), 123 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index f583d502ecc..9c54e5fad1f 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -7,7 +7,6 @@ import ( "regexp" "testing" - "github.com/jinzhu/copier" "github.com/mxmCherry/openrtb" "github.com/prebid/prebid-server/adapters" "github.com/stretchr/testify/assert" @@ -115,65 +114,6 @@ func runSpec(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidd testMakeBidsImpl(t, filename, spec, bidder, actualReqs) } -func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { - t.Helper() - - // Save original bidRequest values to assert no data races occur inside MakeRequests latter - deepBidReqCopy := openrtb.BidRequest{} - //copier.CopyWithOption(&deepBidReqCopy, &spec.BidRequest, copier.Option{DeepCopy: true}) - copier.Copy(&deepBidReqCopy, &spec.BidRequest) - shallowBidReqCopy := spec.BidRequest - - // Save original []Imp elements to assert no data races occur inside MakeRequests latter - deepImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp)) - shallowImpCopies := make([]openrtb.Imp, len(spec.BidRequest.Imp)) - for i := 0; i < len(spec.BidRequest.Imp); i++ { - deepImpCopy := openrtb.Imp{} - copier.Copy(&deepImpCopy, &spec.BidRequest.Imp[i]) - deepImpCopies = append(deepImpCopies, deepImpCopy) - - shallowImpCopy := spec.BidRequest.Imp[i] - shallowImpCopies = append(shallowImpCopies, shallowImpCopy) - } - - // Run MakeRequests - actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) - - // Compare MakeRequests actual output versus expected values found in JSON file - assertErrorList(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors) - assertMakeRequestsOutput(t, filename, actualReqs, spec.HttpCalls) - - // Assert no data races occur using original bidRequest copies of references and values - assertNoDataRace(t, &deepBidReqCopy, &shallowBidReqCopy, deepImpCopies, shallowImpCopies) - - return actualReqs -} - -func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) { - t.Helper() - - bidResponses := make([]*adapters.BidderResponse, 0) - var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors)) - - // We should have as many bids as number of adapters.RequestData found in MakeRequests output - for i := 0; i < len(makeRequestsOut); i++ { - // Run MakeBids with JSON refined spec.HttpCalls info that was asserted to match MakeRequests - // output inside testMakeRequestsImpl - thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t)) - - bidsErrs = append(bidsErrs, theseErrs...) - bidResponses = append(bidResponses, thisBidResponse) - } - - // Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors - assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors) - - // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids - for i := 0; i < len(spec.BidResponses); i++ { - assertMakeBidsOutput(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids) - } -} - type testSpec struct { BidRequest openrtb.BidRequest `json:"mockBidRequest"` HttpCalls []httpCall `json:"httpCalls"` @@ -364,81 +304,312 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) } } +// testMakeBidsImpl asserts the results of the bidder MakeRequests implementation against the expected JSON-defined results +// and makes sure no data races occur +func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { + t.Helper() + + deepBidReqCopy, shallowBidReqCopy, err := prepareDataRaceCopies(&spec.BidRequest) + if err != nil { + t.Errorf("Could not create data race test objects. File: %s Error: %v", filename, err) + return nil + } + + // Run MakeRequests + actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) + + // Compare MakeRequests actual output versus expected values found in JSON file + assertErrorList(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors) + assertMakeRequestsOutput(t, filename, actualReqs, spec.HttpCalls) + + // Assert no data races occur using original bidRequest copies of references and values + assertNoDataRace(t, deepBidReqCopy, shallowBidReqCopy, filename) + + return actualReqs +} + +func prepareDataRaceCopies(original *openrtb.BidRequest) (*openrtb.BidRequest, *openrtb.BidRequest, error) { + + // Save original bidRequest values to assert no data races occur inside MakeRequests latter + deepReqCopy, err := deepCopyBidRequest(original) + if err != nil { + return nil, nil, err + } + + // Mocks the shallow copy PBS core provides to adapters + shallowReqCopy := *original + + // PBS core provides adapters a shallow copy of []Imp elements + shallowReqCopy.Imp = nil + for i := 0; i < len(original.Imp); i++ { + shallowImpCopy := original.Imp[i] + shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) + } + + return deepReqCopy, &shallowReqCopy, nil +} + +// deepCopyBidRequest is our own implementation of a deep copy function custom made for an openrtb.BidRequest object +func deepCopyBidRequest(original *openrtb.BidRequest) (*openrtb.BidRequest, error) { + bytes, err := json.Marshal(original) + if err != nil { + return nil, fmt.Errorf("Unable to marshal original bid request: %v", err) + } + + deepCopy := &openrtb.BidRequest{} + err = json.Unmarshal(bytes, deepCopy) + if err != nil { + return nil, fmt.Errorf("Unable to unmarshal original bid request: %v", err) + } + + // Make sure all Ext fields remain perfectly equal after the Marshal and unmarshal calls + deepCopy = deepCopySliceAndExtAdjustments(deepCopy, original) + + return deepCopy, nil +} + +// deepCopySliceAndExtAdjustments is necessary in order to not get false positives when a json +// entry initializes an empty array (such as "format": []) or the shallow copy `Ext` fields differ +// in line breaks or tabs with the deepCopy ones +func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *openrtb.BidRequest) *openrtb.BidRequest { + if len(deepCopy.Ext) > 0 { + deepCopy.Ext = make([]byte, len(original.Ext)) + copy(deepCopy.Ext, original.Ext) + } + + if deepCopy.Site != nil { + if len(deepCopy.Site.Ext) > 0 { + deepCopy.Site.Ext = make([]byte, len(original.Site.Ext)) + copy(deepCopy.Site.Ext, original.Site.Ext) + } + if original.Site.Cat != nil && len(original.Site.Cat) == 0 { + deepCopy.Site.Cat = []string{} + } + if original.Site.SectionCat != nil && len(original.Site.SectionCat) == 0 { + deepCopy.Site.SectionCat = []string{} + } + if original.Site.PageCat != nil && len(original.Site.PageCat) == 0 { + deepCopy.Site.PageCat = []string{} + } + } + + if deepCopy.App != nil { + if len(deepCopy.App.Ext) > 0 { + deepCopy.App.Ext = make([]byte, len(original.App.Ext)) + copy(deepCopy.App.Ext, original.App.Ext) + } + if original.App.Cat != nil && len(original.App.Cat) == 0 { + deepCopy.App.Cat = []string{} + } + if original.App.SectionCat != nil && len(original.App.SectionCat) == 0 { + deepCopy.App.SectionCat = []string{} + } + if original.App.PageCat != nil && len(original.App.PageCat) == 0 { + deepCopy.App.PageCat = []string{} + } + } + + if deepCopy.Device != nil && len(deepCopy.Device.Ext) > 0 { + deepCopy.Device.Ext = make([]byte, len(original.Device.Ext)) + copy(deepCopy.Device.Ext, original.Device.Ext) + } + + if deepCopy.User != nil { + if len(deepCopy.User.Ext) > 0 { + deepCopy.User.Ext = make([]byte, len(original.User.Ext)) + copy(deepCopy.User.Ext, original.User.Ext) + } + if original.User.Data != nil && len(original.User.Data) == 0 { + original.User.Data = []openrtb.Data{} + } + } + + if deepCopy.Source != nil && len(deepCopy.Source.Ext) > 0 { + deepCopy.Source.Ext = make([]byte, len(original.Source.Ext)) + copy(deepCopy.Source.Ext, original.Source.Ext) + } + + if deepCopy.Regs != nil && len(deepCopy.Regs.Ext) > 0 { + deepCopy.Regs.Ext = make([]byte, len(original.Regs.Ext)) + copy(deepCopy.Regs.Ext, original.Regs.Ext) + } + + for i, imp := range deepCopy.Imp { + if len(imp.Ext) > 0 { + imp.Ext = make([]byte, len(original.Imp[i].Ext)) + copy(imp.Ext, original.Imp[i].Ext) + } + if imp.Banner != nil { + if len(imp.Banner.Ext) > 0 { + imp.Banner.Ext = make([]byte, len(original.Imp[i].Banner.Ext)) + copy(imp.Banner.Ext, original.Imp[i].Banner.Ext) + } + if original.Imp[i].Banner.Format != nil && len(original.Imp[i].Banner.Format) == 0 { + imp.Banner.Format = []openrtb.Format{} + } + if original.Imp[i].Banner.BType != nil && len(original.Imp[i].Banner.BType) == 0 { + imp.Banner.BType = []openrtb.BannerAdType{} + } + if original.Imp[i].Banner.BAttr != nil && len(original.Imp[i].Banner.BAttr) == 0 { + imp.Banner.BAttr = []openrtb.CreativeAttribute{} + } + if original.Imp[i].Banner.ExpDir != nil && len(original.Imp[i].Banner.ExpDir) == 0 { + imp.Banner.ExpDir = []openrtb.ExpandableDirection{} + } + if original.Imp[i].Banner.API != nil && len(original.Imp[i].Banner.API) == 0 { + imp.Banner.API = []openrtb.APIFramework{} + } + } + if imp.Video != nil { + if len(imp.Video.Ext) > 0 { + imp.Video.Ext = make([]byte, len(original.Imp[i].Video.Ext)) + copy(imp.Video.Ext, original.Imp[i].Video.Ext) + } + if original.Imp[i].Video.MIMEs != nil && len(original.Imp[i].Video.MIMEs) == 0 { + imp.Video.MIMEs = []string{} + } + if original.Imp[i].Video.Protocols != nil && len(original.Imp[i].Video.Protocols) == 0 { + imp.Video.Protocols = []openrtb.Protocol{} + } + if original.Imp[i].Video.BAttr != nil && len(original.Imp[i].Video.BAttr) == 0 { + imp.Video.BAttr = []openrtb.CreativeAttribute{} + } + if original.Imp[i].Video.PlaybackMethod != nil && len(original.Imp[i].Video.PlaybackMethod) == 0 { + imp.Video.PlaybackMethod = []openrtb.PlaybackMethod{} + } + if original.Imp[i].Video.Delivery != nil && len(original.Imp[i].Video.Delivery) == 0 { + imp.Video.Delivery = []openrtb.ContentDeliveryMethod{} + } + if original.Imp[i].Video.CompanionAd != nil && len(original.Imp[i].Video.CompanionAd) == 0 { + imp.Video.CompanionAd = []openrtb.Banner{} + } + if original.Imp[i].Video.API != nil && len(original.Imp[i].Video.API) == 0 { + imp.Video.API = []openrtb.APIFramework{} + } + if original.Imp[i].Video.CompanionType != nil && len(original.Imp[i].Video.CompanionType) == 0 { + imp.Video.CompanionType = []openrtb.CompanionType{} + } + } + if imp.Audio != nil { + if len(imp.Audio.Ext) > 0 { + imp.Audio.Ext = make([]byte, len(original.Imp[i].Audio.Ext)) + copy(imp.Audio.Ext, original.Imp[i].Audio.Ext) + } + if original.Imp[i].Audio.MIMEs != nil && len(original.Imp[i].Audio.MIMEs) == 0 { + imp.Audio.MIMEs = []string{} + } + if original.Imp[i].Audio.Protocols != nil && len(original.Imp[i].Audio.Protocols) == 0 { + imp.Audio.Protocols = []openrtb.Protocol{} + } + if original.Imp[i].Audio.BAttr != nil && len(original.Imp[i].Audio.BAttr) == 0 { + imp.Audio.BAttr = []openrtb.CreativeAttribute{} + } + if original.Imp[i].Audio.Delivery != nil && len(original.Imp[i].Audio.Delivery) == 0 { + imp.Audio.Delivery = []openrtb.ContentDeliveryMethod{} + } + if original.Imp[i].Audio.CompanionAd != nil && len(original.Imp[i].Audio.CompanionAd) == 0 { + imp.Audio.CompanionAd = []openrtb.Banner{} + } + if original.Imp[i].Audio.API != nil && len(original.Imp[i].Audio.API) == 0 { + imp.Audio.API = []openrtb.APIFramework{} + } + if original.Imp[i].Audio.CompanionType != nil && len(original.Imp[i].Audio.CompanionType) == 0 { + imp.Audio.CompanionType = []openrtb.CompanionType{} + } + } + if imp.Native != nil { + if len(imp.Native.Ext) > 0 { + imp.Native.Ext = make([]byte, len(original.Imp[i].Native.Ext)) + copy(imp.Native.Ext, original.Imp[i].Native.Ext) + } + if original.Imp[i].Native.API != nil && len(original.Imp[i].Native.API) == 0 { + imp.Native.API = []openrtb.APIFramework{} + } + if original.Imp[i].Native.BAttr != nil && len(original.Imp[i].Native.BAttr) == 0 { + imp.Native.BAttr = []openrtb.CreativeAttribute{} + } + } + if imp.PMP != nil { + if len(imp.PMP.Ext) > 0 { + imp.PMP.Ext = make([]byte, len(original.Imp[i].PMP.Ext)) + copy(imp.PMP.Ext, original.Imp[i].PMP.Ext) + } + if original.Imp[i].PMP.Deals != nil && len(original.Imp[i].PMP.Deals) == 0 { + imp.PMP.Deals = []openrtb.Deal{} + } + } + + if len(imp.Metric) > 0 { + for j, metric := range imp.Metric { + if len(metric.Ext) > 0 { + metric.Ext = make([]byte, len(original.Imp[i].Metric[j].Ext)) + copy(metric.Ext, original.Imp[i].Metric[j].Ext) + } + } + } + } + return deepCopy +} + // assertNoDataRace compares the contents of the reference fields found in the original openrtb.BidRequest to their // original values to make sure they were not modified and we are not incurring indata races. In order to assert // no data races occur in the []Imp array, we call assertNoImpsDataRace() -func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidRequestAfter *openrtb.BidRequest, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) { +func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidRequestAfter *openrtb.BidRequest, filename string) { t.Helper() // Assert reference fields were not modified by bidder adapter MakeRequests implementation - assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field") - assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field") - assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field") - assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field") - assert.Equal(t, bidRequestBefore.Source, bidRequestAfter.Source, "Data race in BidRequest.Source field") - assert.Equal(t, bidRequestBefore.Regs, bidRequestAfter.Regs, "Data race in BidRequest.Regs field") - - // Assert slice fields were not modified by bidder adapter MakeRequests implementation - assert.ElementsMatch(t, bidRequestBefore.WSeat, bidRequestAfter.WSeat, "Data race in BidRequest.[]WSeat array") - assert.ElementsMatch(t, bidRequestBefore.BSeat, bidRequestAfter.BSeat, "Data race in BidRequest.[]BSeat array") - assert.ElementsMatch(t, bidRequestBefore.Cur, bidRequestAfter.Cur, "Data race in BidRequest.[]Cur array") - assert.ElementsMatch(t, bidRequestBefore.WLang, bidRequestAfter.WLang, "Data race in BidRequest.[]WLang array") - assert.ElementsMatch(t, bidRequestBefore.BCat, bidRequestAfter.BCat, "Data race in BidRequest.[]BCat array") - assert.ElementsMatch(t, bidRequestBefore.BAdv, bidRequestAfter.BAdv, "Data race in BidRequest.[]BAdv array") - assert.ElementsMatch(t, bidRequestBefore.BApp, bidRequestAfter.BApp, "Data race in BidRequest.[]BApp array") - assert.ElementsMatch(t, bidRequestBefore.Ext, bidRequestAfter.Ext, "Data race in BidRequest.[]Ext array") + assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field in file %s", filename) + assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field in file %s", filename) + assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field in file %s", filename) + assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field in file %s", filename) + 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 Imps separately - //assertNoImpsDataRace(t, bidRequestBefore.Imp, impsAfter) - assertNoImpsDataRace(t, impsBefore, impsAfter) + assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) } // assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to // their original values to make sure they were not modified and we are not incurring in data races. -func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp) { +func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp, filename string) { + t.Helper() + + if assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called. File:%s", filename) { + // Assert no data races occured in individual Imp elements + for i := 0; i < len(impsBefore); i++ { + assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field. File:%s", i, filename) + assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field. File:%s", i, filename) + assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field. File:%s", i, filename) + 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) + } + } +} + +// testMakeBidsImpl asserts the results of the bidder MakeBids implementation against the expected JSON-defined results +func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) { t.Helper() - assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called") + bidResponses := make([]*adapters.BidderResponse, 0) + var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors)) - // Assert no data races occured in individual Imp elements - for i := 0; i < len(impsBefore); i++ { - assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field", i) - assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field", i) - assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field", i) - assert.Equal(t, impsBefore[i].Native, impsAfter[i].Native, "Data race in bidRequest.Imp[%d].Native field", i) - assert.Equal(t, impsBefore[i].PMP, impsAfter[i].PMP, "Data race in bidRequest.Imp[%d].PMP field", i) - assert.Equal(t, impsBefore[i].Secure, impsAfter[i].Secure, "Data race in bidRequest.Imp[%d].Secure field", i) + // We should have as many bids as number of adapters.RequestData found in MakeRequests output + for i := 0; i < len(makeRequestsOut); i++ { + // Run MakeBids with JSON refined spec.HttpCalls info that was asserted to match MakeRequests + // output inside testMakeRequestsImpl + thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t)) - assert.ElementsMatch(t, impsBefore[i].Metric, impsAfter[i].Metric, "Data race in bidRequest.Imp[%d].[]Metric array", i) - assert.ElementsMatch(t, impsBefore[i].IframeBuster, impsAfter[i].IframeBuster, "Data race in bidRequest.Imp[%d].[]IframeBuster array", i) + bidsErrs = append(bidsErrs, theseErrs...) + bidResponses = append(bidResponses, thisBidResponse) } -} -// func TestACopy() { -// app := openrtb.App{ -// ID: "anyID", -// Name: "anyName", -// Bundle: "anyBundle", -// Domain: "anyDomain", -// StoreURL: "anyUrl.com", -// Cat: []string{"AAA", "BBB"}, -// SectionCat: []string{"aaa", "bbb"}, -// PageCat: []string{"CCC", "DDD"}, -// Ver: "VER", -// PrivacyPolicy: 1, -// Paid: 2, -// Publisher: &openrtb.Publisher{ -// ID: "anyPublisherID", -// Name: "anyPublisherName", -// Cat: []string{"pubAAA", "pubBBB"}, -// Domain: "anyPublisherDomain", -// Ext: json.RawMessage(`{"appPublisherExtField":"value"}`), -// }, -// Content: &openrtb.Content{ -// ID: "anyAppContentID", -// Episode: 1, -// }, -// Keywords: "AnyKeyword", -// Ext: json.RawMessage(`{"appExtField":"value"}`), -// } -// } + // Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors + assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors) + + // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids + for i := 0; i < len(spec.BidResponses); i++ { + assertMakeBidsOutput(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids) + } +} diff --git a/go.mod b/go.mod index b2125269e4d..48fc6b6479b 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,6 @@ require ( github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b github.com/hashicorp/hcl v1.0.0 // indirect github.com/influxdata/influxdb v1.6.1 - github.com/jinzhu/copier v0.2.8 github.com/julienschmidt/httprouter v1.1.0 github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 // indirect github.com/kr/pretty v0.2.0 // indirect diff --git a/go.sum b/go.sum index 10f4fbda8c8..6da3f8898ba 100644 --- a/go.sum +++ b/go.sum @@ -44,8 +44,6 @@ github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= github.com/influxdata/influxdb v1.6.1 h1:OseoBlzI5ftNI/bczyxSWq6PKRCNEeiXvyWP/wS5fB0= github.com/influxdata/influxdb v1.6.1/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY= -github.com/jinzhu/copier v0.2.8 h1:N8MbL5niMwE3P4dOwurJixz5rMkKfujmMRFmAanSzWE= -github.com/jinzhu/copier v0.2.8/go.mod h1:24xnZezI2Yqac9J61UC6/dG/k76ttpq0DdJI3QmUvro= github.com/julienschmidt/httprouter v1.1.0 h1:7wLdtIiIpzOkC9u6sXOozpBauPdskj3ru4EI5MABq68= github.com/julienschmidt/httprouter v1.1.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 h1:uC1QfSlInpQF+M0ao65imhwqKnz3Q2z/d8PWZRMQvDM= From 8dc32b29c8b8869a058417e97d1f30b7162869b9 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Wed, 24 Mar 2021 21:55:07 -0400 Subject: [PATCH 05/15] merge recent modifications to adapters/adapterstest/test_json.go --- adapters/adapterstest/test_json.go | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 9c54e5fad1f..1e94faba90c 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -214,14 +214,29 @@ func assertErrorList(t *testing.T, description string, actual []error, expected } } -func assertMakeBidsOutput(t *testing.T, filename string, actual []*adapters.TypedBid, expected []expectedBid) { +func assertMakeBidsOutput(t *testing.T, filename string, actualBidderResp *adapters.BidderResponse, expected []expectedBid) { t.Helper() - if len(actual) != len(expected) { - t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actual)) + if (actualBidderResp == nil || len(actualBidderResp.Bids) == 0) != (len(expected) == 0) { + if len(expected) == 0 { + t.Fatalf("%s: expectedBidResponses indicated a nil response, but mockResponses supplied a non-nil response", filename) + } + + t.Fatalf("%s: mockResponses included unexpected nil or empty response", filename) } - for i := 0; i < len(actual); i++ { - diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actual[i], &(expected[i])) + + // Expected nil response - give diffBids something to work with. + if actualBidderResp == nil { + actualBidderResp = new(adapters.BidderResponse) + } + + actualBids := actualBidderResp.Bids + + if len(actualBids) != len(expected) { + t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actualBids)) + } + for i := 0; i < len(actualBids); i++ { + diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actualBids[i], &(expected[i])) } } @@ -610,6 +625,6 @@ func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adap // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids for i := 0; i < len(spec.BidResponses); i++ { - assertMakeBidsOutput(t, filename, bidResponses[i].Bids, spec.BidResponses[i].Bids) + assertMakeBidsOutput(t, filename, bidResponses[i], spec.BidResponses[i].Bids) } } From 015fc30c51ee4dc557c01ed389a8ddf2497757d0 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 25 Mar 2021 00:16:26 -0400 Subject: [PATCH 06/15] informative comment --- adapters/adapterstest/test_json.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 1e94faba90c..4f059e182bd 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -230,13 +230,11 @@ func assertMakeBidsOutput(t *testing.T, filename string, actualBidderResp *adapt actualBidderResp = new(adapters.BidderResponse) } - actualBids := actualBidderResp.Bids - - if len(actualBids) != len(expected) { - t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actualBids)) + if len(actualBidderResp.Bids) != len(expected) { + t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actualBidderResp.Bids)) } - for i := 0; i < len(actualBids); i++ { - diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actualBids[i], &(expected[i])) + for i := 0; i < len(actualBidderResp.Bids); i++ { + diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actualBidderResp.Bids[i], &(expected[i])) } } @@ -377,7 +375,8 @@ func deepCopyBidRequest(original *openrtb.BidRequest) (*openrtb.BidRequest, erro return nil, fmt.Errorf("Unable to unmarshal original bid request: %v", err) } - // Make sure all Ext fields remain perfectly equal after the Marshal and unmarshal calls + // Make sure all Ext fields don't differ in blank spaces, line brakes or tabs + // and slices don't get differentiated for being empty versus being nil deepCopy = deepCopySliceAndExtAdjustments(deepCopy, original) return deepCopy, nil From 4ce81b9c86d7996e21219b191deb4b86e53cf9c6 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Fri, 26 Mar 2021 15:57:17 -0400 Subject: [PATCH 07/15] resolved openrtb2 merge conflicts --- adapters/adapterstest/test_json.go | 79 ++++++++++++++++++------------ 1 file changed, 47 insertions(+), 32 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 672d3bf2585..eb104caac10 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -84,7 +84,14 @@ func loadFile(filename string) (*testSpec, error) { return nil, fmt.Errorf("Failed to read file %s: %v", filename, err) } + //var buf bytes.Buffer + + //if err := json.Compact(&buf, specData); err != nil { + // return nil, fmt.Errorf("Unable to compact JSON string found in file %s: %v", filename, err) + //} + var spec testSpec + //if err := json.Unmarshal(buf.Bytes(), &spec); err != nil { if err := json.Unmarshal(specData, &spec); err != nil { return nil, fmt.Errorf("Failed to unmarshal JSON from file: %v", err) } @@ -341,7 +348,7 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder return actualReqs } -func prepareDataRaceCopies(original *openrtb.BidRequest) (*openrtb.BidRequest, *openrtb.BidRequest, error) { +func prepareDataRaceCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest, error) { // Save original bidRequest values to assert no data races occur inside MakeRequests latter deepReqCopy, err := deepCopyBidRequest(original) @@ -362,14 +369,14 @@ func prepareDataRaceCopies(original *openrtb.BidRequest) (*openrtb.BidRequest, * return deepReqCopy, &shallowReqCopy, nil } -// deepCopyBidRequest is our own implementation of a deep copy function custom made for an openrtb.BidRequest object -func deepCopyBidRequest(original *openrtb.BidRequest) (*openrtb.BidRequest, error) { +// deepCopyBidRequest is our own implementation of a deep copy function custom made for an openrtb2.BidRequest object +func deepCopyBidRequest(original *openrtb2.BidRequest) (*openrtb2.BidRequest, error) { bytes, err := json.Marshal(original) if err != nil { return nil, fmt.Errorf("Unable to marshal original bid request: %v", err) } - deepCopy := &openrtb.BidRequest{} + deepCopy := &openrtb2.BidRequest{} err = json.Unmarshal(bytes, deepCopy) if err != nil { return nil, fmt.Errorf("Unable to unmarshal original bid request: %v", err) @@ -385,7 +392,7 @@ func deepCopyBidRequest(original *openrtb.BidRequest) (*openrtb.BidRequest, erro // deepCopySliceAndExtAdjustments is necessary in order to not get false positives when a json // entry initializes an empty array (such as "format": []) or the shallow copy `Ext` fields differ // in line breaks or tabs with the deepCopy ones -func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *openrtb.BidRequest) *openrtb.BidRequest { +func deepCopySliceAndExtAdjustments(deepCopy *openrtb2.BidRequest, original *openrtb2.BidRequest) *openrtb2.BidRequest { if len(deepCopy.Ext) > 0 { deepCopy.Ext = make([]byte, len(original.Ext)) copy(deepCopy.Ext, original.Ext) @@ -433,8 +440,16 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open deepCopy.User.Ext = make([]byte, len(original.User.Ext)) copy(deepCopy.User.Ext, original.User.Ext) } - if original.User.Data != nil && len(original.User.Data) == 0 { - original.User.Data = []openrtb.Data{} + if original.User.Data != nil { + if len(original.User.Data) == 0 { + original.User.Data = []openrtb2.Data{} + } + for i := 0; i < len(deepCopy.User.Data); i++ { + if len(deepCopy.User.Data[i].Ext) > 0 { + deepCopy.User.Data[i].Ext = make([]byte, len(original.User.Data[i].Ext)) + copy(deepCopy.User.Data[i].Ext, original.User.Data[i].Ext) + } + } } } @@ -459,19 +474,19 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open copy(imp.Banner.Ext, original.Imp[i].Banner.Ext) } if original.Imp[i].Banner.Format != nil && len(original.Imp[i].Banner.Format) == 0 { - imp.Banner.Format = []openrtb.Format{} + imp.Banner.Format = []openrtb2.Format{} } if original.Imp[i].Banner.BType != nil && len(original.Imp[i].Banner.BType) == 0 { - imp.Banner.BType = []openrtb.BannerAdType{} + imp.Banner.BType = []openrtb2.BannerAdType{} } if original.Imp[i].Banner.BAttr != nil && len(original.Imp[i].Banner.BAttr) == 0 { - imp.Banner.BAttr = []openrtb.CreativeAttribute{} + imp.Banner.BAttr = []openrtb2.CreativeAttribute{} } if original.Imp[i].Banner.ExpDir != nil && len(original.Imp[i].Banner.ExpDir) == 0 { - imp.Banner.ExpDir = []openrtb.ExpandableDirection{} + imp.Banner.ExpDir = []openrtb2.ExpandableDirection{} } if original.Imp[i].Banner.API != nil && len(original.Imp[i].Banner.API) == 0 { - imp.Banner.API = []openrtb.APIFramework{} + imp.Banner.API = []openrtb2.APIFramework{} } } if imp.Video != nil { @@ -483,25 +498,25 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open imp.Video.MIMEs = []string{} } if original.Imp[i].Video.Protocols != nil && len(original.Imp[i].Video.Protocols) == 0 { - imp.Video.Protocols = []openrtb.Protocol{} + imp.Video.Protocols = []openrtb2.Protocol{} } if original.Imp[i].Video.BAttr != nil && len(original.Imp[i].Video.BAttr) == 0 { - imp.Video.BAttr = []openrtb.CreativeAttribute{} + imp.Video.BAttr = []openrtb2.CreativeAttribute{} } if original.Imp[i].Video.PlaybackMethod != nil && len(original.Imp[i].Video.PlaybackMethod) == 0 { - imp.Video.PlaybackMethod = []openrtb.PlaybackMethod{} + imp.Video.PlaybackMethod = []openrtb2.PlaybackMethod{} } if original.Imp[i].Video.Delivery != nil && len(original.Imp[i].Video.Delivery) == 0 { - imp.Video.Delivery = []openrtb.ContentDeliveryMethod{} + imp.Video.Delivery = []openrtb2.ContentDeliveryMethod{} } if original.Imp[i].Video.CompanionAd != nil && len(original.Imp[i].Video.CompanionAd) == 0 { - imp.Video.CompanionAd = []openrtb.Banner{} + imp.Video.CompanionAd = []openrtb2.Banner{} } if original.Imp[i].Video.API != nil && len(original.Imp[i].Video.API) == 0 { - imp.Video.API = []openrtb.APIFramework{} + imp.Video.API = []openrtb2.APIFramework{} } if original.Imp[i].Video.CompanionType != nil && len(original.Imp[i].Video.CompanionType) == 0 { - imp.Video.CompanionType = []openrtb.CompanionType{} + imp.Video.CompanionType = []openrtb2.CompanionType{} } } if imp.Audio != nil { @@ -513,22 +528,22 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open imp.Audio.MIMEs = []string{} } if original.Imp[i].Audio.Protocols != nil && len(original.Imp[i].Audio.Protocols) == 0 { - imp.Audio.Protocols = []openrtb.Protocol{} + imp.Audio.Protocols = []openrtb2.Protocol{} } if original.Imp[i].Audio.BAttr != nil && len(original.Imp[i].Audio.BAttr) == 0 { - imp.Audio.BAttr = []openrtb.CreativeAttribute{} + imp.Audio.BAttr = []openrtb2.CreativeAttribute{} } if original.Imp[i].Audio.Delivery != nil && len(original.Imp[i].Audio.Delivery) == 0 { - imp.Audio.Delivery = []openrtb.ContentDeliveryMethod{} + imp.Audio.Delivery = []openrtb2.ContentDeliveryMethod{} } if original.Imp[i].Audio.CompanionAd != nil && len(original.Imp[i].Audio.CompanionAd) == 0 { - imp.Audio.CompanionAd = []openrtb.Banner{} + imp.Audio.CompanionAd = []openrtb2.Banner{} } if original.Imp[i].Audio.API != nil && len(original.Imp[i].Audio.API) == 0 { - imp.Audio.API = []openrtb.APIFramework{} + imp.Audio.API = []openrtb2.APIFramework{} } if original.Imp[i].Audio.CompanionType != nil && len(original.Imp[i].Audio.CompanionType) == 0 { - imp.Audio.CompanionType = []openrtb.CompanionType{} + imp.Audio.CompanionType = []openrtb2.CompanionType{} } } if imp.Native != nil { @@ -537,10 +552,10 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open copy(imp.Native.Ext, original.Imp[i].Native.Ext) } if original.Imp[i].Native.API != nil && len(original.Imp[i].Native.API) == 0 { - imp.Native.API = []openrtb.APIFramework{} + imp.Native.API = []openrtb2.APIFramework{} } if original.Imp[i].Native.BAttr != nil && len(original.Imp[i].Native.BAttr) == 0 { - imp.Native.BAttr = []openrtb.CreativeAttribute{} + imp.Native.BAttr = []openrtb2.CreativeAttribute{} } } if imp.PMP != nil { @@ -549,7 +564,7 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open copy(imp.PMP.Ext, original.Imp[i].PMP.Ext) } if original.Imp[i].PMP.Deals != nil && len(original.Imp[i].PMP.Deals) == 0 { - imp.PMP.Deals = []openrtb.Deal{} + imp.PMP.Deals = []openrtb2.Deal{} } } @@ -565,10 +580,10 @@ func deepCopySliceAndExtAdjustments(deepCopy *openrtb.BidRequest, original *open return deepCopy } -// assertNoDataRace compares the contents of the reference fields found in the original openrtb.BidRequest to their +// assertNoDataRace compares the contents of the reference fields found in the original openrtb2.BidRequest to their // original values to make sure they were not modified and we are not incurring indata races. In order to assert // no data races occur in the []Imp array, we call assertNoImpsDataRace() -func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidRequestAfter *openrtb.BidRequest, filename string) { +func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRequestAfter *openrtb2.BidRequest, filename string) { t.Helper() // Assert reference fields were not modified by bidder adapter MakeRequests implementation @@ -583,9 +598,9 @@ func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb.BidRequest, bidReq assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) } -// assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb.Imp objects to +// assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb2.Imp objects to // their original values to make sure they were not modified and we are not incurring in data races. -func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb.Imp, impsAfter []openrtb.Imp, filename string) { +func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb2.Imp, impsAfter []openrtb2.Imp, filename string) { t.Helper() if assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called. File:%s", filename) { From eb75ee446b12a821a8036cdaa0ed3c1a3da79192 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 8 Apr 2021 01:42:41 -0400 Subject: [PATCH 08/15] Rolled back to use the github.com/jinzhu/copier library --- adapters/adapterstest/test_json.go | 239 ++--------------------------- go.mod | 1 + go.sum | 2 + 3 files changed, 17 insertions(+), 225 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index eb104caac10..04d3330f7d6 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -7,6 +7,7 @@ import ( "regexp" "testing" + "github.com/jinzhu/copier" "github.com/mxmCherry/openrtb/v14/openrtb2" "github.com/prebid/prebid-server/adapters" "github.com/stretchr/testify/assert" @@ -329,11 +330,7 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { t.Helper() - deepBidReqCopy, shallowBidReqCopy, err := prepareDataRaceCopies(&spec.BidRequest) - if err != nil { - t.Errorf("Could not create data race test objects. File: %s Error: %v", filename, err) - return nil - } + deepBidReqCopy, shallowBidReqCopy := getDataRaceTestCopies(&spec.BidRequest) // Run MakeRequests actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) @@ -348,236 +345,28 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder return actualReqs } -func prepareDataRaceCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest, error) { +func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest) { // Save original bidRequest values to assert no data races occur inside MakeRequests latter - deepReqCopy, err := deepCopyBidRequest(original) - if err != nil { - return nil, nil, err - } + deepReqCopy := openrtb2.BidRequest{} + copier.Copy(&deepReqCopy, original) - // Mocks the shallow copy PBS core provides to adapters + // Shallow copy PBS core provides to adapters shallowReqCopy := *original - // PBS core provides adapters a shallow copy of []Imp elements - shallowReqCopy.Imp = nil + // Save original []Imp elements to assert no data races occur inside MakeRequests latter + deepReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) + shallowReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) for i := 0; i < len(original.Imp); i++ { + deepImpCopy := openrtb2.Imp{} + copier.Copy(&deepImpCopy, original.Imp[i]) + deepReqCopy.Imp = append(deepReqCopy.Imp, deepImpCopy) + shallowImpCopy := original.Imp[i] shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) } - return deepReqCopy, &shallowReqCopy, nil -} - -// deepCopyBidRequest is our own implementation of a deep copy function custom made for an openrtb2.BidRequest object -func deepCopyBidRequest(original *openrtb2.BidRequest) (*openrtb2.BidRequest, error) { - bytes, err := json.Marshal(original) - if err != nil { - return nil, fmt.Errorf("Unable to marshal original bid request: %v", err) - } - - deepCopy := &openrtb2.BidRequest{} - err = json.Unmarshal(bytes, deepCopy) - if err != nil { - return nil, fmt.Errorf("Unable to unmarshal original bid request: %v", err) - } - - // Make sure all Ext fields don't differ in blank spaces, line brakes or tabs - // and slices don't get differentiated for being empty versus being nil - deepCopy = deepCopySliceAndExtAdjustments(deepCopy, original) - - return deepCopy, nil -} - -// deepCopySliceAndExtAdjustments is necessary in order to not get false positives when a json -// entry initializes an empty array (such as "format": []) or the shallow copy `Ext` fields differ -// in line breaks or tabs with the deepCopy ones -func deepCopySliceAndExtAdjustments(deepCopy *openrtb2.BidRequest, original *openrtb2.BidRequest) *openrtb2.BidRequest { - if len(deepCopy.Ext) > 0 { - deepCopy.Ext = make([]byte, len(original.Ext)) - copy(deepCopy.Ext, original.Ext) - } - - if deepCopy.Site != nil { - if len(deepCopy.Site.Ext) > 0 { - deepCopy.Site.Ext = make([]byte, len(original.Site.Ext)) - copy(deepCopy.Site.Ext, original.Site.Ext) - } - if original.Site.Cat != nil && len(original.Site.Cat) == 0 { - deepCopy.Site.Cat = []string{} - } - if original.Site.SectionCat != nil && len(original.Site.SectionCat) == 0 { - deepCopy.Site.SectionCat = []string{} - } - if original.Site.PageCat != nil && len(original.Site.PageCat) == 0 { - deepCopy.Site.PageCat = []string{} - } - } - - if deepCopy.App != nil { - if len(deepCopy.App.Ext) > 0 { - deepCopy.App.Ext = make([]byte, len(original.App.Ext)) - copy(deepCopy.App.Ext, original.App.Ext) - } - if original.App.Cat != nil && len(original.App.Cat) == 0 { - deepCopy.App.Cat = []string{} - } - if original.App.SectionCat != nil && len(original.App.SectionCat) == 0 { - deepCopy.App.SectionCat = []string{} - } - if original.App.PageCat != nil && len(original.App.PageCat) == 0 { - deepCopy.App.PageCat = []string{} - } - } - - if deepCopy.Device != nil && len(deepCopy.Device.Ext) > 0 { - deepCopy.Device.Ext = make([]byte, len(original.Device.Ext)) - copy(deepCopy.Device.Ext, original.Device.Ext) - } - - if deepCopy.User != nil { - if len(deepCopy.User.Ext) > 0 { - deepCopy.User.Ext = make([]byte, len(original.User.Ext)) - copy(deepCopy.User.Ext, original.User.Ext) - } - if original.User.Data != nil { - if len(original.User.Data) == 0 { - original.User.Data = []openrtb2.Data{} - } - for i := 0; i < len(deepCopy.User.Data); i++ { - if len(deepCopy.User.Data[i].Ext) > 0 { - deepCopy.User.Data[i].Ext = make([]byte, len(original.User.Data[i].Ext)) - copy(deepCopy.User.Data[i].Ext, original.User.Data[i].Ext) - } - } - } - } - - if deepCopy.Source != nil && len(deepCopy.Source.Ext) > 0 { - deepCopy.Source.Ext = make([]byte, len(original.Source.Ext)) - copy(deepCopy.Source.Ext, original.Source.Ext) - } - - if deepCopy.Regs != nil && len(deepCopy.Regs.Ext) > 0 { - deepCopy.Regs.Ext = make([]byte, len(original.Regs.Ext)) - copy(deepCopy.Regs.Ext, original.Regs.Ext) - } - - for i, imp := range deepCopy.Imp { - if len(imp.Ext) > 0 { - imp.Ext = make([]byte, len(original.Imp[i].Ext)) - copy(imp.Ext, original.Imp[i].Ext) - } - if imp.Banner != nil { - if len(imp.Banner.Ext) > 0 { - imp.Banner.Ext = make([]byte, len(original.Imp[i].Banner.Ext)) - copy(imp.Banner.Ext, original.Imp[i].Banner.Ext) - } - if original.Imp[i].Banner.Format != nil && len(original.Imp[i].Banner.Format) == 0 { - imp.Banner.Format = []openrtb2.Format{} - } - if original.Imp[i].Banner.BType != nil && len(original.Imp[i].Banner.BType) == 0 { - imp.Banner.BType = []openrtb2.BannerAdType{} - } - if original.Imp[i].Banner.BAttr != nil && len(original.Imp[i].Banner.BAttr) == 0 { - imp.Banner.BAttr = []openrtb2.CreativeAttribute{} - } - if original.Imp[i].Banner.ExpDir != nil && len(original.Imp[i].Banner.ExpDir) == 0 { - imp.Banner.ExpDir = []openrtb2.ExpandableDirection{} - } - if original.Imp[i].Banner.API != nil && len(original.Imp[i].Banner.API) == 0 { - imp.Banner.API = []openrtb2.APIFramework{} - } - } - if imp.Video != nil { - if len(imp.Video.Ext) > 0 { - imp.Video.Ext = make([]byte, len(original.Imp[i].Video.Ext)) - copy(imp.Video.Ext, original.Imp[i].Video.Ext) - } - if original.Imp[i].Video.MIMEs != nil && len(original.Imp[i].Video.MIMEs) == 0 { - imp.Video.MIMEs = []string{} - } - if original.Imp[i].Video.Protocols != nil && len(original.Imp[i].Video.Protocols) == 0 { - imp.Video.Protocols = []openrtb2.Protocol{} - } - if original.Imp[i].Video.BAttr != nil && len(original.Imp[i].Video.BAttr) == 0 { - imp.Video.BAttr = []openrtb2.CreativeAttribute{} - } - if original.Imp[i].Video.PlaybackMethod != nil && len(original.Imp[i].Video.PlaybackMethod) == 0 { - imp.Video.PlaybackMethod = []openrtb2.PlaybackMethod{} - } - if original.Imp[i].Video.Delivery != nil && len(original.Imp[i].Video.Delivery) == 0 { - imp.Video.Delivery = []openrtb2.ContentDeliveryMethod{} - } - if original.Imp[i].Video.CompanionAd != nil && len(original.Imp[i].Video.CompanionAd) == 0 { - imp.Video.CompanionAd = []openrtb2.Banner{} - } - if original.Imp[i].Video.API != nil && len(original.Imp[i].Video.API) == 0 { - imp.Video.API = []openrtb2.APIFramework{} - } - if original.Imp[i].Video.CompanionType != nil && len(original.Imp[i].Video.CompanionType) == 0 { - imp.Video.CompanionType = []openrtb2.CompanionType{} - } - } - if imp.Audio != nil { - if len(imp.Audio.Ext) > 0 { - imp.Audio.Ext = make([]byte, len(original.Imp[i].Audio.Ext)) - copy(imp.Audio.Ext, original.Imp[i].Audio.Ext) - } - if original.Imp[i].Audio.MIMEs != nil && len(original.Imp[i].Audio.MIMEs) == 0 { - imp.Audio.MIMEs = []string{} - } - if original.Imp[i].Audio.Protocols != nil && len(original.Imp[i].Audio.Protocols) == 0 { - imp.Audio.Protocols = []openrtb2.Protocol{} - } - if original.Imp[i].Audio.BAttr != nil && len(original.Imp[i].Audio.BAttr) == 0 { - imp.Audio.BAttr = []openrtb2.CreativeAttribute{} - } - if original.Imp[i].Audio.Delivery != nil && len(original.Imp[i].Audio.Delivery) == 0 { - imp.Audio.Delivery = []openrtb2.ContentDeliveryMethod{} - } - if original.Imp[i].Audio.CompanionAd != nil && len(original.Imp[i].Audio.CompanionAd) == 0 { - imp.Audio.CompanionAd = []openrtb2.Banner{} - } - if original.Imp[i].Audio.API != nil && len(original.Imp[i].Audio.API) == 0 { - imp.Audio.API = []openrtb2.APIFramework{} - } - if original.Imp[i].Audio.CompanionType != nil && len(original.Imp[i].Audio.CompanionType) == 0 { - imp.Audio.CompanionType = []openrtb2.CompanionType{} - } - } - if imp.Native != nil { - if len(imp.Native.Ext) > 0 { - imp.Native.Ext = make([]byte, len(original.Imp[i].Native.Ext)) - copy(imp.Native.Ext, original.Imp[i].Native.Ext) - } - if original.Imp[i].Native.API != nil && len(original.Imp[i].Native.API) == 0 { - imp.Native.API = []openrtb2.APIFramework{} - } - if original.Imp[i].Native.BAttr != nil && len(original.Imp[i].Native.BAttr) == 0 { - imp.Native.BAttr = []openrtb2.CreativeAttribute{} - } - } - if imp.PMP != nil { - if len(imp.PMP.Ext) > 0 { - imp.PMP.Ext = make([]byte, len(original.Imp[i].PMP.Ext)) - copy(imp.PMP.Ext, original.Imp[i].PMP.Ext) - } - if original.Imp[i].PMP.Deals != nil && len(original.Imp[i].PMP.Deals) == 0 { - imp.PMP.Deals = []openrtb2.Deal{} - } - } - - if len(imp.Metric) > 0 { - for j, metric := range imp.Metric { - if len(metric.Ext) > 0 { - metric.Ext = make([]byte, len(original.Imp[i].Metric[j].Ext)) - copy(metric.Ext, original.Imp[i].Metric[j].Ext) - } - } - } - } - return deepCopy + return &deepReqCopy, &shallowReqCopy } // assertNoDataRace compares the contents of the reference fields found in the original openrtb2.BidRequest to their diff --git a/go.mod b/go.mod index 67f95057bee..7162e65087a 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b github.com/hashicorp/hcl v1.0.0 // indirect github.com/influxdata/influxdb v1.6.1 + github.com/jinzhu/copier v0.2.8 github.com/julienschmidt/httprouter v1.1.0 github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 // indirect github.com/lib/pq v1.0.0 diff --git a/go.sum b/go.sum index f9c9bb887da..dab93e817f4 100644 --- a/go.sum +++ b/go.sum @@ -44,6 +44,8 @@ github.com/hpcloud/tail v1.0.0 h1:nfCOvKYfkgYP8hkirhJocXT2+zOD8yUNjXaWfTlyFKI= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= github.com/influxdata/influxdb v1.6.1 h1:OseoBlzI5ftNI/bczyxSWq6PKRCNEeiXvyWP/wS5fB0= github.com/influxdata/influxdb v1.6.1/go.mod h1:qZna6X/4elxqT3yI9iZYdZrWWdeFOOprn86kgg4+IzY= +github.com/jinzhu/copier v0.2.8 h1:N8MbL5niMwE3P4dOwurJixz5rMkKfujmMRFmAanSzWE= +github.com/jinzhu/copier v0.2.8/go.mod h1:24xnZezI2Yqac9J61UC6/dG/k76ttpq0DdJI3QmUvro= github.com/julienschmidt/httprouter v1.1.0 h1:7wLdtIiIpzOkC9u6sXOozpBauPdskj3ru4EI5MABq68= github.com/julienschmidt/httprouter v1.1.0/go.mod h1:SYymIcj16QtmaHHD7aYtjjsJG7VTCxuUUipMqKk8s4w= github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 h1:uC1QfSlInpQF+M0ao65imhwqKnz3Q2z/d8PWZRMQvDM= From 87d8461203dce069d620f3996c925bcdce44d9ef Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Fri, 16 Apr 2021 11:54:39 -0400 Subject: [PATCH 09/15] step out real quick --- adapters/adapterstest/test_json.go | 7 --- endpoints/openrtb2/auction_test.go | 85 ++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 04d3330f7d6..28d5424cf32 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -85,14 +85,7 @@ func loadFile(filename string) (*testSpec, error) { return nil, fmt.Errorf("Failed to read file %s: %v", filename, err) } - //var buf bytes.Buffer - - //if err := json.Compact(&buf, specData); err != nil { - // return nil, fmt.Errorf("Unable to compact JSON string found in file %s: %v", filename, err) - //} - var spec testSpec - //if err := json.Unmarshal(buf.Bytes(), &spec); err != nil { if err := json.Unmarshal(specData, &spec); err != nil { return nil, fmt.Errorf("Failed to unmarshal JSON from file: %v", err) } diff --git a/endpoints/openrtb2/auction_test.go b/endpoints/openrtb2/auction_test.go index 2deb5d1b762..b271a8290f5 100644 --- a/endpoints/openrtb2/auction_test.go +++ b/endpoints/openrtb2/auction_test.go @@ -20,6 +20,8 @@ import ( "github.com/mxmCherry/openrtb/v14/openrtb2" "github.com/prebid/prebid-server/stored_requests" + "github.com/jinzhu/copier" + "github.com/buger/jsonparser" jsonpatch "github.com/evanphx/json-patch" analyticsConf "github.com/prebid/prebid-server/analytics/config" @@ -51,6 +53,89 @@ type testConfigValues struct { DisabledAdapters []string `json:"disabledAdapters"` } +func TestDeepVsRegCopy(t *testing.T) { + bidRequest := &openrtb2.BidRequest{ + ID: "some-request-id", + Imp: []openrtb2.Imp{ + { + ID: "impression-1", + Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 300, H: 250}, {W: 300, H: 600}}}, + Ext: json.RawMessage(`{"appnexus": {"placementId": 1}}`), + }, + { + ID: "impression-2", + Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 1080, H: 780}, {W: 300, H: 600}}}, + Ext: json.RawMessage(`{"appnexus": {"placementId": 2}}`), + }, + }, + Site: &openrtb2.Site{Page: "prebid.org", Ext: json.RawMessage(`{"amp":0}`)}, + Device: &openrtb2.Device{UA: "curl/7.54.0", IP: "::1"}, + AT: 1, + TMax: 500, + Ext: json.RawMessage(`{"id": "some-request-id","site": {"page": "prebid.org"},"imp": [{"id": "some-impression-id","banner": {"format": [{"w": 300,"h": 250},{"w": 300,"h": 600}]},"ext": {"appnexus": {"placementId": 1}}}],"tmax": 500}`), + } + + deepBidReqCopy, shallowBidReqCopy := getDataRaceTestCopies(bidRequest) + + assertNoDataRace(t, deepBidReqCopy, shallowBidReqCopy, "none") +} + +func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest) { + + // Save original bidRequest values to assert no data races occur inside MakeRequests latter + deepReqCopy := openrtb2.BidRequest{} + copier.Copy(&deepReqCopy, original) + + // Shallow copy PBS core provides to adapters + shallowReqCopy := *original + + // Save original []Imp elements to assert no data races occur inside MakeRequests latter + deepReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) + shallowReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) + for i := 0; i < len(original.Imp); i++ { + deepImpCopy := openrtb2.Imp{} + copier.Copy(&deepImpCopy, original.Imp[i]) + deepReqCopy.Imp = append(deepReqCopy.Imp, deepImpCopy) + + shallowImpCopy := original.Imp[i] + shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) + } + + return &deepReqCopy, &shallowReqCopy +} + +func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRequestAfter *openrtb2.BidRequest, filename string) { + t.Helper() + + // Assert reference fields were not modified by bidder adapter MakeRequests implementation + assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field in file %s", filename) + assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field in file %s", filename) + assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field in file %s", filename) + assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field in file %s", filename) + 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 Imps separately + assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) +} + +func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb2.Imp, impsAfter []openrtb2.Imp, filename string) { + t.Helper() + + if assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called. File:%s", filename) { + // Assert no data races occured in individual Imp elements + for i := 0; i < len(impsBefore); i++ { + assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field. File:%s", i, filename) + assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field. File:%s", i, filename) + assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field. File:%s", i, filename) + 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) + } + } +} + func TestJsonSampleRequests(t *testing.T) { testSuites := []struct { description string From 4e81fcf983cd21ee3db6d134dca036c860486f5c Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 29 Apr 2021 15:26:12 -0400 Subject: [PATCH 10/15] Scott's review and deep copy library switch --- adapters/adapterstest/test_json.go | 65 +++++++++++------------ endpoints/openrtb2/auction_test.go | 85 ------------------------------ go.mod | 2 +- go.sum | 4 ++ 4 files changed, 37 insertions(+), 119 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 28d5424cf32..1603952cf61 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -7,7 +7,7 @@ import ( "regexp" "testing" - "github.com/jinzhu/copier" + "github.com/mohae/deepcopy" "github.com/mxmCherry/openrtb/v14/openrtb2" "github.com/prebid/prebid-server/adapters" "github.com/stretchr/testify/assert" @@ -110,9 +110,9 @@ func runSpec(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidd reqInfo.PbsEntryPoint = "video" } - actualReqs := testMakeRequestsImpl(t, filename, spec, bidder, &reqInfo) + requests := testMakeRequestsImpl(t, filename, spec, bidder, &reqInfo) - testMakeBidsImpl(t, filename, spec, bidder, actualReqs) + testMakeBidsImpl(t, filename, spec, bidder, requests) } type testSpec struct { @@ -215,10 +215,10 @@ func assertErrorList(t *testing.T, description string, actual []error, expected } } -func assertMakeBidsOutput(t *testing.T, filename string, actualBidderResp *adapters.BidderResponse, expected []expectedBid) { +func assertMakeBidsOutput(t *testing.T, filename string, bidderResponse *adapters.BidderResponse, expected []expectedBid) { t.Helper() - if (actualBidderResp == nil || len(actualBidderResp.Bids) == 0) != (len(expected) == 0) { + if (bidderResponse == nil || len(bidderResponse.Bids) == 0) != (len(expected) == 0) { if len(expected) == 0 { t.Fatalf("%s: expectedBidResponses indicated a nil response, but mockResponses supplied a non-nil response", filename) } @@ -227,15 +227,15 @@ func assertMakeBidsOutput(t *testing.T, filename string, actualBidderResp *adapt } // Expected nil response - give diffBids something to work with. - if actualBidderResp == nil { - actualBidderResp = new(adapters.BidderResponse) + if bidderResponse == nil { + bidderResponse = new(adapters.BidderResponse) } - if len(actualBidderResp.Bids) != len(expected) { - t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(actualBidderResp.Bids)) + if len(bidderResponse.Bids) != len(expected) { + t.Fatalf("%s: MakeBids returned wrong bid count. Expected %d, got %d", filename, len(expected), len(bidderResponse.Bids)) } - for i := 0; i < len(actualBidderResp.Bids); i++ { - diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), actualBidderResp.Bids[i], &(expected[i])) + for i := 0; i < len(bidderResponse.Bids); i++ { + diffBids(t, fmt.Sprintf("%s: typedBid[%d]", filename, i), bidderResponse.Bids[i], &(expected[i])) } } @@ -318,53 +318,52 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) } } -// testMakeBidsImpl asserts the results of the bidder MakeRequests implementation against the expected JSON-defined results -// and makes sure no data races occur +// testMakeBidsImpl 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 { t.Helper() deepBidReqCopy, shallowBidReqCopy := getDataRaceTestCopies(&spec.BidRequest) // Run MakeRequests - actualReqs, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) + requests, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) // Compare MakeRequests actual output versus expected values found in JSON file assertErrorList(t, fmt.Sprintf("%s: MakeRequests", filename), errs, spec.MakeRequestErrors) - assertMakeRequestsOutput(t, filename, actualReqs, spec.HttpCalls) + assertMakeRequestsOutput(t, filename, requests, spec.HttpCalls) // Assert no data races occur using original bidRequest copies of references and values assertNoDataRace(t, deepBidReqCopy, shallowBidReqCopy, filename) - return actualReqs + return requests } +// getDataRaceTestCopies returns a deep copy and a shallow copy of the original bidRequest that will get +// 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 func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest) { + deepReqCopy := deepcopy.Copy(original).(*openrtb2.BidRequest) - // Save original bidRequest values to assert no data races occur inside MakeRequests latter - deepReqCopy := openrtb2.BidRequest{} - copier.Copy(&deepReqCopy, original) - - // Shallow copy PBS core provides to adapters shallowReqCopy := *original - - // Save original []Imp elements to assert no data races occur inside MakeRequests latter - deepReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) - shallowReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) + shallowReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) for i := 0; i < len(original.Imp); i++ { - deepImpCopy := openrtb2.Imp{} - copier.Copy(&deepImpCopy, original.Imp[i]) - deepReqCopy.Imp = append(deepReqCopy.Imp, deepImpCopy) - shallowImpCopy := original.Imp[i] shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) } - return &deepReqCopy, &shallowReqCopy + return deepReqCopy, &shallowReqCopy } -// assertNoDataRace compares the contents of the reference fields found in the original openrtb2.BidRequest to their -// original values to make sure they were not modified and we are not incurring indata races. In order to assert -// no data races occur in the []Imp array, we call assertNoImpsDataRace() +// assertNoDataRace compares the contents of the reference fields found in the original openrtb2.BidRequest such as Site, +// App, Device and so on, to their original shared-memory values to make sure they were not modified and we are not incurring +// in data races. Because some adapters modify the lenght of the []Imp array, we call assertNoImpsDataRace() to assert we are +// data-race free there. This function is necessary because a simple `assert.Equalf()` call would also compare non shared +// memory fields that adapters are free to modify, therefore leading us to false positives in terms of data-race detection. func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRequestAfter *openrtb2.BidRequest, filename string) { t.Helper() diff --git a/endpoints/openrtb2/auction_test.go b/endpoints/openrtb2/auction_test.go index b271a8290f5..2deb5d1b762 100644 --- a/endpoints/openrtb2/auction_test.go +++ b/endpoints/openrtb2/auction_test.go @@ -20,8 +20,6 @@ import ( "github.com/mxmCherry/openrtb/v14/openrtb2" "github.com/prebid/prebid-server/stored_requests" - "github.com/jinzhu/copier" - "github.com/buger/jsonparser" jsonpatch "github.com/evanphx/json-patch" analyticsConf "github.com/prebid/prebid-server/analytics/config" @@ -53,89 +51,6 @@ type testConfigValues struct { DisabledAdapters []string `json:"disabledAdapters"` } -func TestDeepVsRegCopy(t *testing.T) { - bidRequest := &openrtb2.BidRequest{ - ID: "some-request-id", - Imp: []openrtb2.Imp{ - { - ID: "impression-1", - Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 300, H: 250}, {W: 300, H: 600}}}, - Ext: json.RawMessage(`{"appnexus": {"placementId": 1}}`), - }, - { - ID: "impression-2", - Banner: &openrtb2.Banner{Format: []openrtb2.Format{{W: 1080, H: 780}, {W: 300, H: 600}}}, - Ext: json.RawMessage(`{"appnexus": {"placementId": 2}}`), - }, - }, - Site: &openrtb2.Site{Page: "prebid.org", Ext: json.RawMessage(`{"amp":0}`)}, - Device: &openrtb2.Device{UA: "curl/7.54.0", IP: "::1"}, - AT: 1, - TMax: 500, - Ext: json.RawMessage(`{"id": "some-request-id","site": {"page": "prebid.org"},"imp": [{"id": "some-impression-id","banner": {"format": [{"w": 300,"h": 250},{"w": 300,"h": 600}]},"ext": {"appnexus": {"placementId": 1}}}],"tmax": 500}`), - } - - deepBidReqCopy, shallowBidReqCopy := getDataRaceTestCopies(bidRequest) - - assertNoDataRace(t, deepBidReqCopy, shallowBidReqCopy, "none") -} - -func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest) { - - // Save original bidRequest values to assert no data races occur inside MakeRequests latter - deepReqCopy := openrtb2.BidRequest{} - copier.Copy(&deepReqCopy, original) - - // Shallow copy PBS core provides to adapters - shallowReqCopy := *original - - // Save original []Imp elements to assert no data races occur inside MakeRequests latter - deepReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) - shallowReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) - for i := 0; i < len(original.Imp); i++ { - deepImpCopy := openrtb2.Imp{} - copier.Copy(&deepImpCopy, original.Imp[i]) - deepReqCopy.Imp = append(deepReqCopy.Imp, deepImpCopy) - - shallowImpCopy := original.Imp[i] - shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) - } - - return &deepReqCopy, &shallowReqCopy -} - -func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRequestAfter *openrtb2.BidRequest, filename string) { - t.Helper() - - // Assert reference fields were not modified by bidder adapter MakeRequests implementation - assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field in file %s", filename) - assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field in file %s", filename) - assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field in file %s", filename) - assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field in file %s", filename) - 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 Imps separately - assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) -} - -func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb2.Imp, impsAfter []openrtb2.Imp, filename string) { - t.Helper() - - if assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called. File:%s", filename) { - // Assert no data races occured in individual Imp elements - for i := 0; i < len(impsBefore); i++ { - assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field. File:%s", i, filename) - assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field. File:%s", i, filename) - assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field. File:%s", i, filename) - 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) - } - } -} - func TestJsonSampleRequests(t *testing.T) { testSuites := []struct { description string diff --git a/go.mod b/go.mod index 7162e65087a..bd529ba3345 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,6 @@ require ( github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b github.com/hashicorp/hcl v1.0.0 // indirect github.com/influxdata/influxdb v1.6.1 - github.com/jinzhu/copier v0.2.8 github.com/julienschmidt/httprouter v1.1.0 github.com/k0kubun/colorstring v0.0.0-20150214042306-9440f1994b88 // indirect github.com/lib/pq v1.0.0 @@ -29,6 +28,7 @@ require ( github.com/mattn/go-colorable v0.1.2 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect github.com/mitchellh/mapstructure v1.0.0 // indirect + github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 github.com/mxmCherry/openrtb/v14 v14.0.0 github.com/onsi/ginkgo v1.10.1 // indirect github.com/onsi/gomega v1.7.0 // indirect diff --git a/go.sum b/go.sum index dab93e817f4..543f37ae9c7 100644 --- a/go.sum +++ b/go.sum @@ -62,6 +62,10 @@ github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0j github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= github.com/mitchellh/mapstructure v1.0.0 h1:vVpGvMXJPqSDh2VYHF7gsfQj8Ncx+Xw5Y1KHeTRY+7I= github.com/mitchellh/mapstructure v1.0.0/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= +github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 h1:RWengNIwukTxcDr9M+97sNutRR1RKhG96O6jWumTTnw= +github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826/go.mod h1:TaXosZuwdSHYgviHp1DAtfrULt5eUgsSMsZf+YrPgl8= +github.com/mxmCherry/openrtb v1.1.0 h1:6/5IvwJNsZOEc/mtD6APlqKffValjFSGLP+TqcZWcBM= +github.com/mxmCherry/openrtb v13.0.0+incompatible h1:KEnUHAJFovM54lcicyjiJgZZ4mWKNw+h0GOJXWOoJ4A= github.com/mxmCherry/openrtb/v14 v14.0.0 h1:7CUpdQi6Hqi9k03AaUAZVYF3Kqt6ZXryBUFQv8IA/N8= github.com/mxmCherry/openrtb/v14 v14.0.0/go.mod h1:aAj4RuDpol+zMEVyKiDqiXPHfXevLVmyAf3f6BkRaJw= github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= From e8d3a6a5ee94141458a2c520fd502e8bde5d9f98 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Wed, 5 May 2021 03:14:16 -0400 Subject: [PATCH 11/15] Added slice assertions --- adapters/adapterstest/test_json.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index af5ab2a4ccc..26ad4247971 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -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 { @@ -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) @@ -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 + 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") + // Assert Imps separately assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) } @@ -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) } } } From 6be78a1bca407c355522554612de29ac05f0b19c Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 6 May 2021 03:00:09 -0400 Subject: [PATCH 12/15] Major corrections. Removed the assertNoDataRace functions entirely --- adapters/adapterstest/test_json.go | 102 ++++++++++------------------- go.mod | 1 + go.sum | 4 ++ 3 files changed, 38 insertions(+), 69 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 26ad4247971..a3951d0628c 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -7,7 +7,7 @@ import ( "regexp" "testing" - "github.com/mohae/deepcopy" + "github.com/mitchellh/copystructure" "github.com/mxmCherry/openrtb/v15/openrtb2" "github.com/prebid/prebid-server/adapters" "github.com/stretchr/testify/assert" @@ -318,13 +318,26 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) } } -// 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 +// testMakeRequestsImpl asserts the resulting values of the bidder's `MakeRequests()` implementation +// against the expected JSON-defined results and makes sure we do not incurr in data races in the +// process. To verify no data races occur this function creates: +// 1) A shallow copy of the unmarshalled openrtb2.BidRequest that will provide reference values to +// shared memory that we don't want the adapters' implementation of `MakeRequests()` to modify. +// 2) A deep copy that will preserve the original values of all the fields. This copy remains untouched +// by the adapters' processes and serves as reference of what the shared memory values should still +// be after the `MakeRequests()` call. +// +// The original values stored in the deep copy will be compared against the shared memory values and +// discrepancies will point to data race conditions in an adapter's `MakeRequests()` implementation. +// +// Because neither the shallow nor the deep copies are passed to `MakeRequests()` we know static fields +// in openrtb2.BidRequest nor openrtb2.Imp elements will change, therefore, we can reliably compare +// reference values using the `assert.Equal()` method. func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { t.Helper() - deepBidReqCopy, shallowBidReqCopy := getDataRaceTestCopies(&spec.BidRequest) + deepBidReqCopy, shallowBidReqCopy, err := getDataRaceTestCopies(&spec.BidRequest) + assert.NoError(t, err, "Could not create deep copy for data race assertions. %s\n", filename) // Run MakeRequests requests, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) @@ -334,79 +347,30 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder assertMakeRequestsOutput(t, filename, requests, spec.HttpCalls) // Assert no data races occur using original bidRequest copies of references and values - assertNoDataRace(t, deepBidReqCopy, shallowBidReqCopy, filename) + assert.Equal(t, deepBidReqCopy, shallowBidReqCopy, "Data race found. Test: %s \n", filename) return requests } // getDataRaceTestCopies returns a deep copy and a shallow copy of the original bidRequest that will get -// 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 (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) - - shallowReqCopy := *original - shallowReqCopy.Imp = make([]openrtb2.Imp, 0, len(original.Imp)) - for i := 0; i < len(original.Imp); i++ { - shallowImpCopy := original.Imp[i] - shallowReqCopy.Imp = append(shallowReqCopy.Imp, shallowImpCopy) +// compared to verify no data races occur. +func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, *openrtb2.BidRequest, error) { + cpy, err := copystructure.Copy(original) + if err != nil { + return nil, nil, err } + deepReqCopy := cpy.(*openrtb2.BidRequest) - return deepReqCopy, &shallowReqCopy -} - -// assertNoDataRace compares the contents of the reference fields found in the original openrtb2.BidRequest such as Site, -// App, Device and so on, to their original shared-memory values to make sure they were not modified and we are not incurring -// in data races. Because some adapters modify the lenght of the []Imp array, we call assertNoImpsDataRace() to assert we are -// data-race free there. This function is necessary because a simple `assert.Equalf()` call would also compare non shared -// memory fields that adapters are free to modify, therefore leading us to false positives in terms of data-race detection. -func assertNoDataRace(t *testing.T, bidRequestBefore *openrtb2.BidRequest, bidRequestAfter *openrtb2.BidRequest, filename string) { - t.Helper() - - // Assert reference fields were not modified by bidder adapter MakeRequests implementation - assert.Equal(t, bidRequestBefore.Site, bidRequestAfter.Site, "Data race in BidRequest.Site field in file %s", filename) - assert.Equal(t, bidRequestBefore.App, bidRequestAfter.App, "Data race in BidRequest.App field in file %s", filename) - assert.Equal(t, bidRequestBefore.Device, bidRequestAfter.Device, "Data race in BidRequest.Device field in file %s", filename) - assert.Equal(t, bidRequestBefore.User, bidRequestAfter.User, "Data race in BidRequest.User field in file %s", filename) - 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 - 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") - - // Assert Imps separately - assertNoImpsDataRace(t, bidRequestBefore.Imp, bidRequestAfter.Imp, filename) -} - -// assertNoImpsDataRace compares the contents of the reference fields found in the original openrtb2.Imp objects to -// their original values to make sure they were not modified and we are not incurring in data races. -func assertNoImpsDataRace(t *testing.T, impsBefore []openrtb2.Imp, impsAfter []openrtb2.Imp, filename string) { - t.Helper() + shallowReqCopy := *original - if assert.Len(t, impsAfter, len(impsBefore), "Original []Imp array was modified and length is not equal to original after MakeRequests was called. File:%s", filename) { - // Assert no data races occured in individual Imp elements - for i := 0; i < len(impsBefore); i++ { - assert.Equal(t, impsBefore[i].Banner, impsAfter[i].Banner, "Data race in bidRequest.Imp[%d].Banner field. File:%s", i, filename) - assert.Equal(t, impsBefore[i].Video, impsAfter[i].Video, "Data race in bidRequest.Imp[%d].Video field. File:%s", i, filename) - assert.Equal(t, impsBefore[i].Audio, impsAfter[i].Audio, "Data race in bidRequest.Imp[%d].Audio field. File:%s", i, filename) - 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.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) - } + // If we call `copy` on a nil []Imp, the shallowReqCopy.Imp slice will initialize to openrtb2.Imp{} + // and not nil, which will lead to false positives. + if original.Imp != nil { + shallowReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) + copy(shallowReqCopy.Imp, original.Imp) } + + return deepReqCopy, &shallowReqCopy, nil } // testMakeBidsImpl asserts the results of the bidder MakeBids implementation against the expected JSON-defined results diff --git a/go.mod b/go.mod index 51ac20dd6da..e241b8ecb17 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( github.com/magiconair/properties v1.8.0 github.com/mattn/go-colorable v0.1.2 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect + github.com/mitchellh/copystructure v1.1.2 github.com/mitchellh/mapstructure v1.0.0 // indirect github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 github.com/mxmCherry/openrtb/v15 v15.0.0 diff --git a/go.sum b/go.sum index baf3bf68e68..cbf743fe798 100644 --- a/go.sum +++ b/go.sum @@ -73,8 +73,12 @@ github.com/mattn/go-isatty v0.0.8 h1:HLtExJ+uU2HOZ+wI0Tt5DtUDrx8yhUqDcp7fYERX4CE github.com/mattn/go-isatty v0.0.8/go.mod h1:Iq45c/XA43vh69/j3iqttzPXn0bhXyGjM0Hdxcsrc5s= github.com/matttproud/golang_protobuf_extensions v1.0.1 h1:4hp9jkHxhMHkqkrB3Ix0jegS5sx/RkqARlsWZ6pIwiU= github.com/matttproud/golang_protobuf_extensions v1.0.1/go.mod h1:D8He9yQNgCq6Z5Ld7szi9bcBfOoFv/3dc6xSMkL2PC0= +github.com/mitchellh/copystructure v1.1.2 h1:Th2TIvG1+6ma3e/0/bopBKohOTY7s4dA8V2q4EUcBJ0= +github.com/mitchellh/copystructure v1.1.2/go.mod h1:EBArHfARyrSWO/+Wyr9zwEkc6XMFB9XyNgFNmRkZZU4= github.com/mitchellh/mapstructure v1.0.0 h1:vVpGvMXJPqSDh2VYHF7gsfQj8Ncx+Xw5Y1KHeTRY+7I= github.com/mitchellh/mapstructure v1.0.0/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= +github.com/mitchellh/reflectwalk v1.0.1 h1:FVzMWA5RllMAKIdUSC8mdWo3XtwoecrH79BY70sEEpE= +github.com/mitchellh/reflectwalk v1.0.1/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 h1:RWengNIwukTxcDr9M+97sNutRR1RKhG96O6jWumTTnw= github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826/go.mod h1:TaXosZuwdSHYgviHp1DAtfrULt5eUgsSMsZf+YrPgl8= github.com/mxmCherry/openrtb/v15 v15.0.0 h1:inLuQ3Bsima9HLB2v6WjbtEFF69SWOT5Dux4QZtYdrw= From 1023f300764454b2c53147fcd2e8a6546ae62eaa Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 6 May 2021 03:13:16 -0400 Subject: [PATCH 13/15] go mod tidy-ed --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index e241b8ecb17..b7fab89fba1 100644 --- a/go.mod +++ b/go.mod @@ -29,7 +29,6 @@ require ( github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect github.com/mitchellh/copystructure v1.1.2 github.com/mitchellh/mapstructure v1.0.0 // indirect - github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 github.com/mxmCherry/openrtb/v15 v15.0.0 github.com/pelletier/go-toml v1.2.0 // indirect github.com/prebid/go-gdpr v0.8.3 diff --git a/go.sum b/go.sum index cbf743fe798..3ddeaa2f8cc 100644 --- a/go.sum +++ b/go.sum @@ -79,8 +79,6 @@ github.com/mitchellh/mapstructure v1.0.0 h1:vVpGvMXJPqSDh2VYHF7gsfQj8Ncx+Xw5Y1KH github.com/mitchellh/mapstructure v1.0.0/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= github.com/mitchellh/reflectwalk v1.0.1 h1:FVzMWA5RllMAKIdUSC8mdWo3XtwoecrH79BY70sEEpE= github.com/mitchellh/reflectwalk v1.0.1/go.mod h1:mSTlrgnPZtwu0c4WaC2kGObEpuNDbx0jmZXqmk4esnw= -github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 h1:RWengNIwukTxcDr9M+97sNutRR1RKhG96O6jWumTTnw= -github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826/go.mod h1:TaXosZuwdSHYgviHp1DAtfrULt5eUgsSMsZf+YrPgl8= github.com/mxmCherry/openrtb/v15 v15.0.0 h1:inLuQ3Bsima9HLB2v6WjbtEFF69SWOT5Dux4QZtYdrw= github.com/mxmCherry/openrtb/v15 v15.0.0/go.mod h1:TVgncsz6MOzbL7lhun1lNuUBzVBlVDbxf9Fyy1TyhZA= github.com/nxadm/tail v1.4.4/go.mod h1:kenIhsEOeOJmVchQTgglprH7qJGnHDVpk1VPCcaMI8A= From 5f81f1d3c0967642f25390cf9e77f2742d17223c Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Thu, 6 May 2021 17:11:23 -0400 Subject: [PATCH 14/15] Scott's latest review. Improving error messages and reducing large comment --- adapters/adapterstest/test_json.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index a3951d0628c..585d052694e 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -319,25 +319,18 @@ func diffJson(t *testing.T, description string, actual []byte, expected []byte) } // testMakeRequestsImpl asserts the resulting values of the bidder's `MakeRequests()` implementation -// against the expected JSON-defined results and makes sure we do not incurr in data races in the -// process. To verify no data races occur this function creates: +// against the expected JSON-defined results and ensures we do not encounter data races in the process. +// To assert no data races happen we make use of: // 1) A shallow copy of the unmarshalled openrtb2.BidRequest that will provide reference values to // shared memory that we don't want the adapters' implementation of `MakeRequests()` to modify. // 2) A deep copy that will preserve the original values of all the fields. This copy remains untouched // by the adapters' processes and serves as reference of what the shared memory values should still // be after the `MakeRequests()` call. -// -// The original values stored in the deep copy will be compared against the shared memory values and -// discrepancies will point to data race conditions in an adapter's `MakeRequests()` implementation. -// -// Because neither the shallow nor the deep copies are passed to `MakeRequests()` we know static fields -// in openrtb2.BidRequest nor openrtb2.Imp elements will change, therefore, we can reliably compare -// reference values using the `assert.Equal()` method. func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, reqInfo *adapters.ExtraRequestInfo) []*adapters.RequestData { t.Helper() deepBidReqCopy, shallowBidReqCopy, err := getDataRaceTestCopies(&spec.BidRequest) - assert.NoError(t, err, "Could not create deep copy for data race assertions. %s\n", filename) + assert.NoError(t, err, "Could not create request copies. %s", filename) // Run MakeRequests requests, errs := bidder.MakeRequests(&spec.BidRequest, reqInfo) @@ -347,7 +340,7 @@ func testMakeRequestsImpl(t *testing.T, filename string, spec *testSpec, bidder assertMakeRequestsOutput(t, filename, requests, spec.HttpCalls) // Assert no data races occur using original bidRequest copies of references and values - assert.Equal(t, deepBidReqCopy, shallowBidReqCopy, "Data race found. Test: %s \n", filename) + assert.Equal(t, deepBidReqCopy, shallowBidReqCopy, "Data race found. Test: %s", filename) return requests } From d0e5e70c2cb2d76496ad1f29da578a8b584fbdc7 Mon Sep 17 00:00:00 2001 From: Gus Carreon Date: Tue, 11 May 2021 12:09:21 -0400 Subject: [PATCH 15/15] Modified imp shallow copies comment --- adapters/adapterstest/test_json.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/adapters/adapterstest/test_json.go b/adapters/adapterstest/test_json.go index 585d052694e..35a5a57021a 100644 --- a/adapters/adapterstest/test_json.go +++ b/adapters/adapterstest/test_json.go @@ -356,8 +356,9 @@ func getDataRaceTestCopies(original *openrtb2.BidRequest) (*openrtb2.BidRequest, shallowReqCopy := *original - // If we call `copy` on a nil []Imp, the shallowReqCopy.Imp slice will initialize to openrtb2.Imp{} - // and not nil, which will lead to false positives. + // Prebid Server core makes shallow copies of imp elements and adapters are allowed to make changes + // to them. Therefore, we need shallow copies of Imp elements here so our test replicates that + // functionality and only fail when actual shared momory gets modified. if original.Imp != nil { shallowReqCopy.Imp = make([]openrtb2.Imp, len(original.Imp)) copy(shallowReqCopy.Imp, original.Imp)