-
Notifications
You must be signed in to change notification settings - Fork 224
Test: Bidder removal fields #4329
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
base: clean-bidder-request
Are you sure you want to change the base?
Test: Bidder removal fields #4329
Conversation
`<launchContainers>false</launchContainers>`
osulzhenko
left a comment
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.
Additionally, we must ensure that the value is exposed only to the specified bidder. Please add test cases involving multiple bidders to verify fields:
ext.prebid.bidadjustmentfactors
ext.prebid.bidadjustments
ext.prebid.multibid
ext.prebid.alternatebiddercodes
| import org.prebid.server.functional.service.PrebidServerService | ||
| import org.prebid.server.functional.util.HttpUtil | ||
| import org.prebid.server.functional.util.PBSUtils | ||
| import spock.lang.IgnoreRest |
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.
pls remove
| and: "Bidder request should contain ext.prebid.creativeTids" | ||
| assert bidderRequest.ext.prebid.createTids | ||
|
|
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.
privacy package is not the best place to verify this field
| @@ -1,5 +1,6 @@ | |||
| package org.prebid.server.functional.tests | |||
|
|
|||
| import org.prebid.server.functional.model.bidderspecific.BidderRequest | |||
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.
dead import
| def bidderRequest = bidder.getBidderRequest(bidRequest.id) | ||
| assert !bidderRequest.user.ext.prebid.buyeruids | ||
|
|
||
| and: "Bidder request should contain fcapid,time,consentedProvidedSettings" |
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.
and: "Bidder request should contain user.ext.{fcapid,time,consentedProvidersSettings}"
|
|
||
| then: "Bidder request should contain requested amp" | ||
| def bidderRequest = bidder.getBidderRequest(bidRequest.id) | ||
| assert bidderRequest.ext.prebid.amp |
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.
assert bidderRequest.ext.prebid.amp == bidRequest.ext.prebid.amp
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.
same for others
| key = "custom_bid_request" | ||
| source = "bidrequest" | ||
| value = "imp.id" |
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.
make it constant or variables or random values, it's looks like magic numbers
| assert bidderRequest.ext.prebid.trace | ||
| assert bidderRequest.ext.prebid.debug | ||
| assert bidderRequest.ext.prebid.integration |
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.
replace with verifyAll and confirm that equals to bidRequest
| assert bidderRequest.ext.prebid.amp | ||
| } | ||
|
|
||
| def "PBS shouldn't pass ext.prebid.analytics to bidder request when analytics specified"() { |
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.
we can use for this check: PBS should populate log analytics with additional data when log is directly non-restricted for account and data specified
| assert bidderRequest.ext.prebid.sdk.renderers.name == bidRequest.ext.prebid.sdk.renderers.name | ||
| assert bidderRequest.ext.prebid.sdk.renderers.version == bidRequest.ext.prebid.sdk.renderers.version | ||
| assert bidderRequest.ext.prebid.sdk.renderers.data.any == bidRequest.ext.prebid.sdk.renderers.data.any |
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.
replace with verifyAll
| assert bidderRequest.user.ext.fcapids == bidRequest.user.ext.fcapids | ||
| assert bidderRequest.user.ext.time == bidRequest.user.ext.time | ||
| assert bidderRequest.user.ext.consentedProvidersSettings == bidRequest.user.ext.consentedProvidersSettings |
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.
verifyAll
…ional-tests/remove-redundat-fields-from-bidderrequest
…ional-tests/remove-redundat-fields-from-bidderrequest
1bb910d to
ea8225a
Compare
…ional-tests/remove-redundat-fields-from-bidderrequest # Conflicts: # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestCleaner.java # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestCurrencyBlocker.java # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestMediaFilter.java # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestPostProcessor.java # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestPreferredMediaProcessor.java # src/main/java/org/prebid/server/auction/bidderrequestpostprocessor/CompositeBidderRequestPostProcessor.java # src/main/java/org/prebid/server/spring/config/ServiceConfiguration.java # src/test/java/org/prebid/server/auction/ExchangeServiceTest.java # src/test/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestCleanerTest.java # src/test/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestCurrencyBlockerTest.java # src/test/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestMediaFilterTest.java # src/test/java/org/prebid/server/auction/bidderrequestpostprocessor/BidderRequestPreferredMediaProcessorTest.java # src/test/java/org/prebid/server/auction/bidderrequestpostprocessor/CompositeBidderRequestPostProcessorTest.java
| import org.prebid.server.functional.model.response.auction.BidResponse | ||
| import org.prebid.server.functional.util.PBSUtils | ||
| import org.prebid.server.functional.util.privacy.CcpaConsent | ||
| import spock.lang.IgnoreRest |
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.
pls remove
| } | ||
| Boolean supportDeals | ||
| String integration | ||
| Map<String, String> bidders |
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.
Never used
| def "PBS should prefer bid price adjustment based on media type when request has per-media-type bid adjustment factors"() { | ||
| given: "Default bid request with bid adjustment" | ||
| def bidAdjustment = randomDecimal | ||
| def bidAdjustment = roundDecimal(getRandomDecimal(), 0) |
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.
Why do we need to round here?
|
|
||
| def "PBS should pass ext.prebid.bidAdjustmentFactors only bidder related entry for each bidder"() { | ||
| given: "Default bid request with bid adjustment" | ||
| def bidAdjustment = roundDecimal(getRandomDecimal(), 0) |
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.
Why do we need to round here?
| assert !bidderRequest?.ext?.prebid?.bidAdjustmentFactors?.mediaTypes[VIDEO] | ||
|
|
||
| where: | ||
| bidAdjustmentFactor << [0.9, 1.1] |
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.
can be change to mediaTypeBidAdjustment
|
|
||
| and: "Bidder request should contain user.ext.{fcapid,time,consentedProvidersSettings}" | ||
| verifyAll(bidderRequest) { | ||
| bidderRequest.user.ext.fcapids == bidRequest.user.ext.fcapids |
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.
verifyAll(bidderRequest.user.ext)
| def "PBS should pass user.ext to bidder request when user.ext specified"() { | ||
| given: "Default basic BidRequest with generic bidder" | ||
| def bidRequest = BidRequest.defaultBidRequest.tap { | ||
| user = new User(ext: new UserExt().tap { |
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.
Define UserExt as def
| given: "Default bid request" | ||
| def bidRequest = BidRequest.defaultBidRequest.tap { | ||
| ext.prebid.bidderConfig = [new ExtPrebidBidderConfig(bidders: [BidderName.GENERIC], config: | ||
| new BidderConfig(ortb2: new BidderConfigOrtb(site: Site.configFPDSite, user: User.configFPDUser)))] |
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.
also can be PrebidBidderConfig
| assert !bidderRequest.ext.prebid.bidderConfig | ||
| } | ||
|
|
||
| def "PBS shouldn't pass bidder param to the bidder when bidder param bidder not requested"() { |
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.
It's not that simple. This field depends on ext.prebid.bidders. Verify that it sends the parameter to the specific bidder and ignores others. Ensure the test reflects the PBS logic for correct field mapping
private static Map<String, JsonNode> bidderToPrebidBidders(BidRequest bidRequest) {
final ExtRequestPrebid prebid = PbsUtil.extRequestPrebid(bidRequest);
final ObjectNode bidders = prebid == null ? null : prebid.getBidders();
if (bidders == null || bidders.isNull()) {
return Collections.emptyMap();
}
final Map<String, JsonNode> bidderToPrebidParameters = new HashMap<>();
final Iterator<Map.Entry<String, JsonNode>> biddersToParams = bidders.fields();
while (biddersToParams.hasNext()) {
final Map.Entry<String, JsonNode> bidderToParam = biddersToParams.next();
bidderToPrebidParameters.put(bidderToParam.getKey(), bidderToParam.getValue());
}
return bidderToPrebidParameters;
final JsonNode prebidParameters = bidderToPrebidBidders.get(bidder);
final ObjectNode bidders = prebidParameters != null
? mapper.mapper().valueToTree(ExtPrebidBidders.of(prebidParameters))
: null;
...
return ExtRequest.of(extPrebidBuilder
.multibid(resolveExtRequestMultiBids(bidderToMultiBid.get(bidder), bidder))
.bidders(bidders)
.bidderparams(prepareBidderParameters(extPrebid, bidder))
private ObjectNode prepareBidderParameters(ExtRequestPrebid prebid, String bidder) {
final ObjectNode bidderParams = prebid != null ? prebid.getBidderparams() : null;
final JsonNode params = bidderParams != null ? bidderParams.get(bidder) : null;
return params != null ? mapper.mapper().createObjectNode().set(bidder, params) : null;
}
| def "PBS shouldn't pass ext.prebid.data.{bidders,eidpermissions} to bidder request"() { | ||
| given: "Default bid request" | ||
| def bidRequest = BidRequest.defaultBidRequest.tap { | ||
| ext.prebid.data = new ExtRequestPrebidData(bidders: [GENERIC.value], |
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.
you are using ErrorType.GENERIC
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
🔎 New Bid Adapter Checklist
🧪 Test plan
How do you know the changes are safe to ship to production?
🏎 Quality check