-
Notifications
You must be signed in to change notification settings - Fork 756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TripleLift Prebid S2S Adapter #954
Conversation
@hhhjort I've added some tests into the triplelifttest/exemplary directory. can you take a look? |
openrtb_ext/bidders.go
Outdated
@@ -135,7 +137,7 @@ func NewBidderParamsValidator(schemaDirectory string) (BidderParamValidator, err | |||
for _, fileInfo := range fileInfos { | |||
bidderName := strings.TrimSuffix(fileInfo.Name(), ".json") | |||
if _, isValid := BidderMap[bidderName]; !isValid { | |||
return nil, fmt.Errorf("File %s/%s does not match a valid BidderName.", schemaDirectory, fileInfo.Name()) | |||
return nil, fmt.Errorf("File %s/%s does not match a valid BidderName.", schemaDirectory, bidderName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change line 140 to
140 return nil, fmt.Errorf("File %s/%s does not match a valid BidderName.", schemaDirectory, fileInfo.Name())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question regarding the banner's width and height properties and: Can we add more test cases so we get a 95% code coverage?
╰─➤ go test -coverprofile=c.out
PASS
coverage: 80.6% of statements
ok github.com/prebid/prebid-server/adapters/triplelift 0.019s
╰─➤ go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:26: getBidType 100.0%
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:34: processImp 76.9%
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:58: MakeRequests 73.9%
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:96: getBidCount 100.0%
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:104: MakeBids 81.0%
github.com/prebid/prebid-server/adapters/triplelift/triplelift.go:144: NewTripleliftBidder 100.0%
github.com/prebid/prebid-server/adapters/triplelift/usersync.go:10: NewTripleliftSyncer 100.0%
total: (statements) 80.6%
will do |
Hey, @guscarreon , it seems that most of the lines not covered by the unit tests cover error conditions, like
Even though it's just a few lines, since these functions are all so short, that they aren't covered by the test procedures reduces code coverage pretty significantly. |
Hi @Kevin-P-Kerr. I believe the EqualError() function is useful for this purpose. Some test cases throughout the code base use it extensively:
|
@guscarreon cumulative test coverage is now 95 percent.
|
Kevin, some Travis errors seem to have popped up with this last update, can we correct them? |
hey @guscarreon I think github might have just froze up or something.
seems like it couldn't access this strechr-objx dependency. Anyway we can retrigger? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kevin-P-Kerr seems like Github was having problems because for some reason the Travis build seems to have built correctly this time around. Thanks for your patience, your adapter looks awesome.
Thanks! What are the next steps in this process? |
} | ||
imp.TagID = tlext.InvCode | ||
// floor is optional | ||
if tlext.Floor == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just have if tlext.Floor != nil { imp.BidFloor = *tlext.Floor }
as you return nil
in either case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point, that's a bit more parsimonious. but it seems this is already merged. if I have to patch later I'll take this idea and apply it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one minor thing, but shouldn't block approval.
@Kevin-P-Kerr : Just noticed that the user sync URL makes no reference to the PBS URL ... there is no way for your usersync endpoint to send the user ID back to the particular PBS installation. |
@hhhjort I'm not totally following. Can you elaborate? What steps should I take to rectify this? |
Look at https://github.com/prebid/prebid-server/blob/master/docs/developers/cookie-syncs.md Basically, your cookie sync endpoint needs to redirect back to the supplied URL and include the user's Triplelift ID so that PBS can populate |
Ok, thanks for the clarification. We'll schedule some time to get a fix in. |
* ignore swp files * start small * start really small * add a user sync * justify * triplelift adapter * add our endpoint * fix syntax * config stuff * compiler fixes * more config * add params * making progress * make our ext more exty * start making responses * more logic * fix compilation errors * can we just nil this out? * augment our json * radically simplify our json * fix errs * infer the bid type * fix syntax * fix comilation errors * rename * fix compilation error * config stuff * simplify params * more config stuff * fixes * revert this * fix up the extension * getting closer * add a test * update config * update bidder params * add the floor here, too * add a usersync test * validation, ws, and a test * update tests * fix test * update email * why not * change email * preprocess requests * do some parsing * take care of some errors * floor is optional * ws * remove native * everything is either banner or video * this should be a float * floor to floor * fix compilation errors * add some tests * more tests * more tests * simplify * more progress * format * ws * rm * don't need this * fix test * fix test * don't ignore swap * change line back * report an error if there are no valid impressions for triplelift * check for either a Banner or Video object on the impression * more tests * mv * more tests
* ignore swp files * start small * start really small * add a user sync * justify * triplelift adapter * add our endpoint * fix syntax * config stuff * compiler fixes * more config * add params * making progress * make our ext more exty * start making responses * more logic * fix compilation errors * can we just nil this out? * augment our json * radically simplify our json * fix errs * infer the bid type * fix syntax * fix comilation errors * rename * fix compilation error * config stuff * simplify params * more config stuff * fixes * revert this * fix up the extension * getting closer * add a test * update config * update bidder params * add the floor here, too * add a usersync test * validation, ws, and a test * update tests * fix test * update email * why not * change email * preprocess requests * do some parsing * take care of some errors * floor is optional * ws * remove native * everything is either banner or video * this should be a float * floor to floor * fix compilation errors * add some tests * more tests * more tests * simplify * more progress * format * ws * rm * don't need this * fix test * fix test * don't ignore swap * change line back * report an error if there are no valid impressions for triplelift * check for either a Banner or Video object on the impression * more tests * mv * more tests
* ignore swp files * start small * start really small * add a user sync * justify * triplelift adapter * add our endpoint * fix syntax * config stuff * compiler fixes * more config * add params * making progress * make our ext more exty * start making responses * more logic * fix compilation errors * can we just nil this out? * augment our json * radically simplify our json * fix errs * infer the bid type * fix syntax * fix comilation errors * rename * fix compilation error * config stuff * simplify params * more config stuff * fixes * revert this * fix up the extension * getting closer * add a test * update config * update bidder params * add the floor here, too * add a usersync test * validation, ws, and a test * update tests * fix test * update email * why not * change email * preprocess requests * do some parsing * take care of some errors * floor is optional * ws * remove native * everything is either banner or video * this should be a float * floor to floor * fix compilation errors * add some tests * more tests * more tests * simplify * more progress * format * ws * rm * don't need this * fix test * fix test * don't ignore swap * change line back * report an error if there are no valid impressions for triplelift * check for either a Banner or Video object on the impression * more tests * mv * more tests
The TripleLift prebid S2S adapter.
request to prebid
request from prebid to tl
response from prebid