ResetDigital: OpenRTB #4475
ResetDigital: OpenRTB #4475
Conversation
*feat: Fix coverage and jsonunit test
Updated ResetDigital adapter tests to use the new 'resetdigitaltest' directory structure. Migrated all test JSON files from 'testdata' to the new format, updated references in the test code, and removed the old testdata files.
*Created new testing files *Deleted old testing files *Removed testing code from adapter *Formated resetdigital_test to use new testing files
This commit standardizes whitespace and formatting. No functional changes were made.
Set the gvlVendorID for Reset Digital in the bidder info YAML file.
- Use len(bidResp.SeatBid) for BidderResponse capacity - Remove redundant currency assignment logic - Maintain backward compatibility with existing tests
Updated ResetDigital adapter tests to use the new 'resetdigitaltest' directory structure. Migrated all test JSON files from 'testdata' to the new format, updated references in the test code, and removed the old testdata files.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
There was a problem hiding this comment.
In the latest commit your adapter adds audio, video and native, but it also removed test cases for video and audio (files adapters/resetdigital/resetdigitaltest/exemplary/simple-audio.json and adapters/resetdigital/resetdigitaltest/exemplary/simple-video.json). Should we add test coverage for these two in addition of native?
- Enforce single impression per request with explicit BadInput error.
- Use `_` for unused params in MakeRequests and MakeBids.
- Map tagid from placement_id when missing.
- Default request currency to USD when empty.
- Build endpoint URI as {endpoint}?pid=<placement_id>.
- parseBidResponse: set currency with fallback (resp.cur → req.cur[0] → USD).
- getBidType: prefer MType; otherwise validate ImpID against request.Imp[0]
and infer media type. Remove redundant length check/loop.
- Filter bids with price <= 0 (Warning), keep seat fallback to "resetdigital".
- JSON samples present: simple-audio.json, simple-video.json, simple-banner.json,
zero-price-bid.json, empty-response.json, multiple-imp-error.json.
- Unit tests:
* TestParseBidResponse_SeatFallback
* TestParseBidResponse_CurrencyFallback_RequestCur
* TestParseBidResponse_CurrencyFallback_DefaultUSD
* TestGetBidType includes Audio MType
* Edge cases for malformed ext, missing placement_id, etc.
Rationale: addresses reviewer feedback on unused params, redundant logic in
getBidType, and missing audio/video coverage; adds currency/seat fallbacks.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
| rdImp.MediaTypes.Video.Mimes = append(rdImp.MediaTypes.Video.Mimes, imp.Video.MIMEs...) | ||
| } | ||
|
|
||
| if len(request.Cur) == 0 || (len(request.Cur) == 1 && request.Cur[0] == "") { |
There was a problem hiding this comment.
Under the current logic, the edge cases below, as unlikely as they probably are, would pass.
func TestFunc(t *testing.T) {
testCases := []struct {
desc string
in []string
expected []string
}{
{
desc: "one char unknown currency",
in: []string{"X"},
expected: []string{"X"},
},
{
desc: "multiple empty strings",
in: []string{"", ""},
expected: []string{"", ""},
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
out := curr(tc.in)
assert.ElementsMatch(t, tc.expected, out)
})
}
}
In order to not overcomplicate 3-letter currency code validation, do you think it'll be convenient to use golang's library ParseISO(string) function?
88 reqCopy.Imp[0].TagID = resetDigitalExt.PlacementID
89 }
90
91 - if len(request.Cur) == 0 || (len(request.Cur) == 1 && request.Cur[0] == "") {
92 - reqCopy.Cur = []string{currencyUSD}
93 - } else {
94 - reqCopy.Cur = request.Cur
95 - }
+ for _, s := range request.Cur {
+ currCode, err := currency.ParseISO(s)
+ if err == nil {
+ reqCopy.Cur = []string{currCode}
+ break
+ }
+ }
+ if len(reqCopy.Cur) == 0 {
+ reqCopy.Cur = []string{currencyUSD}
+ }
96
97 reqBody, err := json.Marshal(&reqCopy)
currency/constant_rates.go
There was a problem hiding this comment.
That’s a great point. I’ll update this to validate with ParseISO, keep only valid codes, and fall back to USD. Thanks!
Implemented ISO-4217 currency validation (ParseISO) with trimming/uppercasing, filtered out invalid entries, and added a safe fallback to USD in both request and response paths. Extended unit tests to cover edge cases and HTTP/JSON handling.
Code coverage summaryNote:
resetdigitalRefer here for heat map coverage report |
There was a problem hiding this comment.
Looks good to me. @Valentino3 thank you for addressing my feedback
| } | ||
| }, | ||
| "required": ["placement_id"], |
There was a problem hiding this comment.
Making a parameter required is a breaking change so we will need to hold this PR until the next major release which is 4.0. 4.0 will is tentatively scheduled to be released next month.
| - native | ||
| - audio | ||
|
|
||
| endpoint: https://prebid.resetdigital.co |
There was a problem hiding this comment.
Nitpick: please move this to the top of the file.
|
|
||
| gvlVendorID: 1162 | ||
|
|
There was a problem hiding this comment.
Nitpick: remove blank lines
| "github.com/prebid/prebid-server/v3/openrtb_ext" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestJsonSamples(t *testing.T) { |
There was a problem hiding this comment.
We require using the JSON test framework to achieve code coverage wherever possible as described in the docs: Adapter Code Tests. The framework performs additional checks that you would not otherwise get with a traditional unit test like ensuring your adapter isn't corrupting shared memory. Unit tests should be used sparingly or not at all as all code coverage is usually attainable via JSON tests.
| if resetDigitalExt.PlacementID == "" { | ||
| return nil, []error{&errortypes.BadInput{ | ||
| Message: "Missing required parameter 'placement_id'", | ||
| }} | ||
| } |
There was a problem hiding this comment.
This check is not necessary since you have tagged placement_id as a required parameter in your bidder-params file which means PBS core will only call your adapter if this parameter is provided.
bsardo
dismissed stale reviews from ccorbo and guscarreon
Test coverage must be performed via JSON test framework wherever possible so will eventually need a re-review once they add that coverage.
New branch for Reset Digital work. All future commits will be here.
Replaces: #4349
Clean branch; continuing Reset Digital updates here going forward.
Supersedes: #4349
Active branch for Reset Digital changes from now on.
Replaces: #4349
New working branch—use this for all future Reset Digital commits.
Supersedes: #4349