Skip to content
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 exchange json tests with correct hb_pb_cat_dur #3836

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

laurb9
Copy link
Contributor

@laurb9 laurb9 commented Aug 1, 2024

Some exchange json tests define the bidVideo response component but do not actually load it, so they validate hb_pb_cat_dur values that are different from what PBS would generate. The difference is that PBS adds _30s when that duration is given in BidVideo, but the tests use _0s.

This change updates bidderBid definition so bidVideo is deserialized into the mock response as BidVideo.
It also updates the json tests with the correct value for hb_pb_cat_dur. Alternatively, we could preserve the current values and remove BidVideo from the tests that don't need it.

A solution to the same issue in adapter tests was proposed in #3835

I am not sure why BidVideo is still necessary though. Bid.Cat[0] and Bid.Dur are available to be passed back in the Bid response, so the targeting code could use them instead.

@bsardo bsardo self-assigned this Sep 3, 2024
@bsardo bsardo assigned VeronikaSolovei9 and hhhjort and unassigned bsardo Sep 9, 2024
@VeronikaSolovei9
Copy link
Contributor

I am not sure why BidVideo is still necessary though. Bid.Cat[0] and Bid.Dur are available to be passed back in the Bid response, so the targeting code could use them instead.

This is up to the adapter to control how to return it. Here is an example of another location usage to fill BidVideo:

BidVideo: &openrtb_ext.ExtBidPrebidVideo{Duration: bidExt.Appnexus.CreativeInfo.Video.Duration},

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bsardo bsardo merged commit c02ee8c into prebid:master Sep 12, 2024
3 checks passed
@laurb9 laurb9 deleted the fix-exchange-tests-bidvideo branch September 12, 2024 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants