New Adapter: Zentotem #4053
New Adapter: Zentotem #4053
Conversation
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
fix after review
fix after review
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
add adapter for zentotem
fix after review
fix after review
replace v2 to v3
# Conflicts: # adapters/zentotem/params_test.go # adapters/zentotem/zentotem.go # adapters/zentotem/zentotem_test.go # exchange/adapter_builders.go
Code coverage summaryNote:
zentotemRefer here for heat map coverage report |
| @@ -0,0 +1,16 @@ | |||
| endpoint: "https://rtb.zentotem.net/bid?sspuid=cqlnvfk00bhs0b6rci6g" | |||
| maintainer: | |||
| email: [email protected] | |||
There was a problem hiding this comment.
I sent email. Please response.
There was a problem hiding this comment.
@przemkaczmarek hello, we sent a response letter for verification
| @@ -0,0 +1,16 @@ | |||
| endpoint: "https://rtb.zentotem.net/bid?sspuid=cqlnvfk00bhs0b6rci6g" | |||
There was a problem hiding this comment.
@przemkaczmarek Hello! This is a correct response because no body was sent in the request.
|
@ccorbo can you please review? |
| for i := range seatBid.Bid { | ||
| bidType, err := getMediaTypeForBid(seatBid.Bid[i]) | ||
| if err != nil { | ||
| errors = append(errors, err) |
There was a problem hiding this comment.
suggestion: you're collecting errors, then not returning the errors below. Can you update to return the errors, or remove this
| @@ -5,6 +5,7 @@ go 1.21 | |||
| retract v3.0.0 // Forgot to update major version in import path and module name | |||
|
|
|||
| require ( | |||
| github.com/51Degrees/device-detection-go/v4 v4.4.35 | |||
There was a problem hiding this comment.
Why were these moved from the indirect section to the direct section?
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderZentotem, config.Adapter{ | ||
| Endpoint: "https://rtb.zentotem.net/bid?sspuid=cqlnvfk00bhs0b6rci6g"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
I suggest using fake urls for your tests for maintenance reasons.
There was a problem hiding this comment.
What is the significance of the numbers in the file names of the exemplary JSON test files?
I think you can get rid of these numbers and some of these tests. For example, I ran a diff on adapters/zentotem/zentotemtest/exemplary/banner-200-212.json and adapters/zentotem/zentotemtest/exemplary/banner-201-202-203.json and they appear to be the same except for id, adid and crid which should not have any impact on your code coverage.
| ) | ||
|
|
||
| //Zentotem doesn't currently require any custom fields. This file is included for conformity only | ||
| //We do include an unused, non-required custom param in static/bidder-params/zentotem.json, but only to hinder the prebid server from crashing by looking for at least 1 custom param |
There was a problem hiding this comment.
It looks like this was copied from another adapter, vrtcal, which was created six years ago. Is a custom param still needed to keep prebid server from crashing? I suspect not; I will investigate.
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "get bid native type", |
There was a problem hiding this comment.
Nitpick: t.Run will substitute underscores for spaces in the test name so we advise using underscores instead of spaces to separate words in name so when a test fails we can simply copy the test name from the output and search for it in the code base.
| endpoint string | ||
| } | ||
|
|
||
| // Builder builds a new instance of the {bidder} adapter for the given bidder with the given config. |
There was a problem hiding this comment.
Nitpick: {bidder} should be Zentotem
There was a problem hiding this comment.
I suggest keeping your tests lean. For example, I don't see much value in having all of those device fields in your tests as they are not fields that are explicitly touched in your adapter. You can probably remove some of them and other pieces of data to reduce the noise.
There was a problem hiding this comment.
Do you need video-300.json and video-301.json? I think you can delete one of these and just have video.json. They are almost identical tests.
There was a problem hiding this comment.
Based on your adapter's behavior and the fact that calls aren't being made to your server during the test suite run, I consider the following tests to basically be the same, in which case I suggest replacing these with a single 204.json test:
adapters/zentotem/zentotemtest/supplemental/banner-empty-width-height-204.json
adapters/zentotem/zentotemtest/supplemental/banner-invalid-width-204.json
adapters/zentotem/zentotemtest/supplemental/banner-invalid-height-204.json
The lone differences here are what the width and height are set to but they have no bearing on the mock response.
The same applies to:
adapters/zentotem/zentotemtest/supplemental/video-empty-protocol-204.json
adapters/zentotem/zentotemtest/supplemental/video-invalid-mimes-204.json
adapters/zentotem/zentotemtest/supplemental/video-without-mimes-204.json
You shouldn't need separate 204 tests for these scenarios. The single 204.json should suffice.
There was a problem hiding this comment.
The three 400 status code tests could probably be combined as well.
Docs: prebid/prebid.github.io#5553