Bug: Assert BidderResponses array in adapter JSON tests

[guscarreon]

This pull request corrects a bug in the adapter JSON test framework. Current version of testMakeBidsImpl compares the expected bid responses with the returned bid responses in the for loop in line 460 shown below:

440 func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) {
441     t.Helper()
442 *-- 14 lines: bidResponses := make([]*adapters.BidderResponse, 0)-----------------------------------------------------------------------
456     // Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors
457     assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors)
458
459     // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids
460     for i := 0; i < len(spec.BidResponses); i++ {
461         assertMakeBidsOutput(t, filename, bidResponses[i], spec.BidResponses[i])
462     }
463 }
adapters/adapterstest/test_json.go

Under the current logic, if a JSON test file comes with an empty "expectedBidResponses" field, no assertions are made even if the bidResponses array was populated with elements. This makes the test framework unable to identify if a given test returns a valid *adapters.BidderResponse when it was not supposed to. To correct this issue, this PR adds the following:

456       // Assert actual errors thrown by MakeBids implementation versus expected JSON-defined spec.MakeBidsErrors
457       assertErrorList(t, fmt.Sprintf("%s: MakeBids", filename), bidsErrs, spec.MakeBidsErrors)
458  
459       // Assert MakeBids implementation BidResponses with expected JSON-defined spec.BidResponses[i].Bids
    +     if !assert.Len(t, bidResponses, len(spec.BidResponses), "%s: MakeBids len(bidResponses) = %d vs len(spec.BidResponses) = %d", filename, len(bidResponses), len(spec.BidResponses)) {
460           for i := 0; i < len(spec.BidResponses); i++ {
461               assertMakeBidsOutput(t, filename, bidResponses[i], spec.BidResponses[i])
462           }
    +     }
463   }
adapters/adapterstest/test_json.go

But a complementary change is needed if we add this check: consider the MakeBids implementation shown below. If a test case exits in line 200, nil values for *adapters.BidderResponse and []error are returned.

197 // MakeBids make the bids for the bid response.
198 func (a *TtxAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
199     if response.StatusCode == http.StatusNoContent {
200         return nil, nil
201     }
202
203     if response.StatusCode == http.StatusBadRequest {
adapters/33across/33across.go

Consequently, the append() calls in lines 452 and 453 of adapters/adapterstest/test_json.go will add the nil references to the bidResponses and bidsErrs arrays which complicates the proposed assert.Len(t, bidResponses, len(spec.BidResponses).. assertion. Therefore, we should prevent nil elements from being added:

440   func testMakeBidsImpl(t *testing.T, filename string, spec *testSpec, bidder adapters.Bidder, makeRequestsOut []*adapters.RequestData) {
441       t.Helper()
442  
443       bidResponses := make([]*adapters.BidderResponse, 0)
444       var bidsErrs = make([]error, 0, len(spec.MakeBidsErrors))
445  
446       // We should have as many bids as number of adapters.RequestData found in MakeRequests output
447       for i := 0; i < len(makeRequestsOut); i++ {
448           // Run MakeBids with JSON refined spec.HttpCalls info that was asserted to match MakeRequests
449           // output inside testMakeRequestsImpl
450           thisBidResponse, theseErrs := bidder.MakeBids(&spec.BidRequest, spec.HttpCalls[i].Request.ToRequestData(t), spec.HttpCalls[i].Response.ToResponseData(t))
451  
    +         if theseErrs != nil {
452               bidsErrs = append(bidsErrs, theseErrs...)
    +         }
    +         if thisBidResponse != nil {
453               bidResponses = append(bidResponses, thisBidResponse)
    +         }
454       }
adapters/adapterstest/test_json.go

With the proposed changes, many JSON test files needed modification. One that is particularly common, is to substitute an empty "expectedBidResponses": [] with "expectedBidResponses": [{"currency":"USD","bids":[]}] like in the adapters/zeta_global_ssp/zeta_global_ssp-test/supplemental/invalid-bid-type.json file below:

116     }
117   ],
118   "expectedBidResponses": [],
119   "expectedMakeBidsErrors": [
120     {
121       "value": "invalid BidType: invalid",
122       "comparison": "literal"
123     }
124   ]
125 }
adapters/zeta_global_ssp/zeta_global_ssp-test/supplemental/invalid-bid-type.json

The reason behind this change is that function NewBidderResponseWithBidsCapacity, that gets called inside MakeBids(...) by every adapter, returns a BidderResponse with an empty, non-nil Bids array and the Currency set to US dollars:

69 // NewBidderResponseWithBidsCapacity create a new BidderResponse initialising the bids array capacity and the default currency value
70 // to "USD".
71 //
72 // bidsCapacity allows to set initial Bids array capacity.
73 // By default, currency is USD but this behavior might be subject to change.
74 func NewBidderResponseWithBidsCapacity(bidsCapacity int) *BidderResponse {
75     return &BidderResponse{
76         Currency: "USD",
77         Bids:     make([]*TypedBid, 0, bidsCapacity),
78     }
79 }
adapters/bidder.go

Please let me know your thoughts.

[VeronikaSolovei9]

Looks good!

[ccorbo]

IX changes LGTM

[maxime-dupuis]

Sharethrough changes LGTM

[vrtcal-dev]

VRTCAL changes LGTM

[wojciech-bialy-wpm]

SSPBC changes LGTM

[braizhas]

ADF changes LGTM.

[el-chuck]

Smaato changes LGTM