New Adapter: Seedtag #4198
New Adapter: Seedtag #4198
Conversation
Code coverage summaryNote:
seedtagRefer here for heat map coverage report |
|
@ccorbo can you give this the first look? |
|
Endpoint returns a 400 - is this expected? 
Confirmed user syncing works correctly: Confirmed GLV ID: Sent email to mainter email address to verify. |
| @@ -0,0 +1,69 @@ | |||
| { | |||
There was a problem hiding this comment.
could you add a test where you receive a bid with an unexpected mtype?
There was a problem hiding this comment.
added this test and also a test for currency conversion errors
|
Confirmed maintainer email: |
…quest on currency conversion error
Code coverage summaryNote:
seedtagRefer here for heat map coverage report |
|
The 400 error is expected if not providing an OpenRtb well formed payload |
|
|
||
| var invalidParams = []string{ | ||
| `{"adUnitId": 123}`, | ||
| `{"adUnitId": ""}`, |
There was a problem hiding this comment.
Since adUnitId is a required param, I suggest adding a case where adUnitId is not provided: {}
adapters/seedtag/seedtag_test.go
Outdated
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderSeedtag, config.Adapter{ | ||
| Endpoint: "https://s.seedtag.com"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
Nitpick: I suggest using a test url for maintenance purposes.
adapters/seedtag/seedtag.go
Outdated
| if responseData.StatusCode == http.StatusNoContent { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if responseData.StatusCode == http.StatusBadRequest { | ||
| return nil, []error{&errortypes.BadInput{ | ||
| Message: fmt.Sprintf("unexpected status code: %d. Run with request.debug = 1 for more info", responseData.StatusCode), | ||
| }} | ||
| } | ||
|
|
||
| if responseData.StatusCode != http.StatusOK { | ||
| return nil, []error{fmt.Errorf("unexpected status code: %d. Run with request.debug = 1 for more info", responseData.StatusCode)} | ||
| } |
There was a problem hiding this comment.
I understand that the prebid docs do not reflect this yet but please use the adapter utility functions adapters/response.go#IsResponseStatusCodeNoContent and adapters/response.go#CheckResponseStatusCodeForErrors to handle invalid status codes.
adapters/seedtag/seedtag.go
Outdated
|
|
||
| requestCopy := *request | ||
| requestCopy.Imp = imps | ||
| requestJSON, err := json.Marshal(requestCopy) |
There was a problem hiding this comment.
Please use jsonutil.Marshal instead of json.Marshal.
adapters/seedtag/seedtag.go
Outdated
| return nil, []error{err} | ||
| } | ||
|
|
||
| bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp)) |
There was a problem hiding this comment.
Nitpick: It might be better to size this based on the number of valid impressions rather than the initial number of impressions. You should be able to access the valid impression count via requestData.ImpIDs.
adapters/seedtag/seedtag.go
Outdated
| case 2: | ||
| return openrtb_ext.BidTypeVideo, nil | ||
| default: | ||
| return "", fmt.Errorf("bid.MType invalid") |
There was a problem hiding this comment.
Nitpick: I suggest errors.New for errors that are just string literals.
adapters/seedtag/seedtag_test.go
Outdated
| adapterstest.RunJSONBidderTest(t, "seedtagtest", bidder) | ||
| } | ||
|
|
||
| func TestGetMediaTypeForBid(t *testing.T) { |
There was a problem hiding this comment.
All code coverage should be achieved via the JSON framework wherever possible. You've achieved full coverage of the GetMediaTypeForBid function through your JSON tests so I don't see this unit test offering much value. I suggest deleting it but you can keep it if you like.
If you decide to keep it, please do the following:
- replace all spaces in your test names with underscores (e.g.
"video mediaType"becomes"video_mediaType" - fix typo "invalie"
- replace body with the following using
t.Runand removingwantErrContain:
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
var bid openrtb2.Bid
bid.MType = openrtb2.MarkupType(test.value)
got, gotErr := getMediaTypeForBid(bid)
assert.Equal(t, test.want, got)
if test.wantErr {
assert.Error(t, gotErr)
}
})
}
| imp.BidFloorCur = "USD" | ||
| imp.BidFloor = convertedValue |
There was a problem hiding this comment.
These two lines are missing test coverage. Please add a supplemental JSON test where a successful currency conversion takes place.
|
|
||
| var response openrtb2.BidResponse | ||
| if err := jsonutil.Unmarshal(responseData.Body, &response); err != nil { | ||
| return nil, []error{err} |
There was a problem hiding this comment.
Please add a supplemental JSON test where the response body unmarshaling fails.
Code coverage summaryNote:
seedtagRefer here for heat map coverage report |
|
Hi @bsardo I've done all changes you pointed out, thanks for the catches ;) |
documentation PR - prebid/prebid.github.io#5866