-
Notifications
You must be signed in to change notification settings - Fork 731
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
Update adapter json test framework to validate BidVideo #3835
Conversation
There are a few tests that do not specify `primary_category` and would have failed; we can update the tests or update the structure. Here I update the structure assuming that the tests show the intention.
Code coverage summaryNote:
adapterstestRefer here for heat map coverage report
|
openrtb_ext/bid.go
Outdated
Duration int `json:"duration,omitempty"` | ||
PrimaryCategory string `json:"primary_category,omitempty"` |
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.
Good catch!
I think we need to fix unit tests instead. Here is why:
- We will not hide known issues in unit tests.
- We will not modify core structures to satisfy unit tests.
- We will have unit tests in this PR that will show how to use this new feature and set an examples for future contributors.
I only had to fix 3 files to make unit tests pass:
/adapters/freewheelssp/freewheelssptest/exemplary/multi-imp.json
/adapters/yeahmobi/yeahmobitest/exemplary/simple-video.json
/adapters/pubmatic/pubmatictest/exemplary/video.json
Change in adapter json tests is expected here. I'll confirm this with the team just in 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.
Thank you, pushed another commit efd1719 to fix adapter tests and remove the change in bid.go
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.
LGTM
Adapter json test framework appears to ignore the
expectedBid.video
object and so the related tests do not test anything.Add
expectedBid.Video
and diff function to validate this part of the adapter response. There are a few tests that only havevideo.duration
and novideo.primary_category
and those would fail; we can update the tests or update theExtPrebidVideo
structure to make them optional.Here I update the structure assuming that the tests show the intention, but if that's not desirable we could update the tests instead.
There's a similar issue with exchange tests that I will look to address separately.
Fixes the existing adapter tests including the one added in #3834 so they actually do validate the expected response.