-
Notifications
You must be signed in to change notification settings - Fork 223
Tests: Support GET interface profiles #4239
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: get-interface
Are you sure you want to change the base?
Conversation
marki1an
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.
Please add validation for get parameters
| @JsonProperty("sarid") | ||
| String storedAuctionResponseId | ||
|
|
||
| @JsonProperty("mimes") |
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.
No, really needed here JsonProperty
| @JsonProperty("oh") | ||
| Integer originalHeight | ||
|
|
||
| @JsonProperty("sizes") |
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 here
| @JsonProperty("ms") | ||
| Object sizesLegacy | ||
|
|
||
| @JsonProperty("slot") |
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 here
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.
Check for similar occurrences
| List<Integer> api | ||
|
|
||
| @JsonProperty("battr") | ||
| List<Integer> battr |
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.
Better naming for it blockAttributes
| Integer podSequence | ||
|
|
||
| @JsonProperty("proto") | ||
| List<Integer> proto |
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 rename to protocols
| and: "Bidder request should contain mimes from param" | ||
| def bidderRequest = bidder.getBidderRequest(request.id) | ||
| assert bidderRequest.imp.size() == 1 | ||
| assert bidderRequest.imp.first.getProperty(impMediaType.value).mimes == [mimes] |
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.
I'd like to propose creating a utility method to get this field, since we will often use it, and it will be faster than using getProperty.
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.
Add in where block null value when mimes not defined.
Comment applicable to all tests
| impMediaType << [ | ||
| MediaType.BANNER, | ||
| MediaType.VIDEO, | ||
| MediaType.AUDIO | ||
| ] |
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.
In one line, please
| it.mimes = [mimes] | ||
| } | ||
|
|
||
| "Default stored request" |
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.
Mino: missing and:
| } | ||
| } | ||
|
|
||
| def "PBS should apply ow and oh for banner imp from general get request when it's 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.
You can merge with PBS should apply width and height for banner imp from general get request when it's specified
| and: "Default bid response" | ||
| def bidResponse = BidResponse.getDefaultBidResponse(request) | ||
| bidder.setResponse(request.id, bidResponse) |
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 can remove bid response since it's present in BaseSpec
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 take a look at this comment
| and: "Default bid response" | ||
| def bidResponse = BidResponse.getDefaultBidResponse(request) | ||
| bidder.setResponse(request.id, bidResponse) |
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 take a look at this comment
| given: "Default General get request" | ||
| def validMemis = PBSUtils.randomString | ||
| def generalGetRequest = GeneralGetRequest.default.tap { | ||
| it.mimes = (invalidMimes + validMemis) |
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.
What about "$invalid $valid"?
|
|
||
| def "PBS should apply width and height for banner imp from general get request when banner width and height are square dimensions"() { | ||
| given: "Default General get request" | ||
| def side = PBSUtils.getRandomNumber(0, 10) |
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 side?
|
|
||
| def "PBS should apply sizes banner imp from general get request when banner width and height are square dimensions"() { | ||
| given: "Default General get request" | ||
| def side = PBSUtils.getRandomNumber(0, 10) |
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 here: Why side
| impMediaType << [ | ||
| MediaType.VIDEO, | ||
| MediaType.AUDIO | ||
| ] |
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.
| impMediaType << [ | |
| MediaType.VIDEO, | |
| MediaType.AUDIO | |
| ] | |
| impMediaType << [MediaType.VIDEO, MediaType.AUDIO] |
| ] | ||
| } | ||
|
|
||
| def "PBS should apply api from general get request when it's 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.
What if not 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.
It would be nice to see all tests with a negative scenario when not specified
| and: "Default bid response" | ||
| def bidResponse = BidResponse.getDefaultBidResponse(request) | ||
| bidder.setResponse(request.id, bidResponse) |
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 remove it
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
| assert bidderRequest.imp.first.video.linearity == linearityParam | ||
| } | ||
|
|
||
| def "PBS should apply minbr from general get request when it's 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.
PBS should apply minbr and maxbr from general get request when it's specified
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should contain boxingallowed from param" |
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.
typo: "playbackmethod"
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should contain boxingallowed from param" |
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 here
| storedRequestDao.save(storedRequest) | ||
|
|
||
| when: "PBS processes general get request" | ||
| def response = pbsWithStoredProfiles.(getRequest) |
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.
.sendGeneralGetRequest
| when: "PBS processes amp request" | ||
| defaultPbsService.sendGeneralGetRequest(generalGetRequest) | ||
|
|
||
| then: "Amp response should contain value from targeting in imp.ext.data" |
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 amp response.
| } | ||
|
|
||
| def "PBS should throw exception when profiles are not configured for filesystem and request contain profileId"() { | ||
| def "PBS should process request when profiles are not configured for filesystem and request contain profileId"() { |
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.
process request with a warning
| import groovy.transform.EqualsAndHashCode | ||
| import groovy.transform.ToString | ||
|
|
||
| @EqualsAndHashCode |
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.
I hope you don't need it
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, check other cases, please remove when it's not really needed
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 need all of them; it's part of the verification process
| import groovy.transform.ToString | ||
| import org.prebid.server.functional.util.PBSUtils | ||
|
|
||
| @EqualsAndHashCode |
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 don't need, since you comparing by fields
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should contain addtlConsent from param" |
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.
"Bidder request should contain regs.gpp from param"
| it.ext == new RegsExt() | ||
| } | ||
|
|
||
| and: "PBS should cansel request" |
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.
typo
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should be valid" |
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.
"PBs should preform bidder request"
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should contain outputformat from param" |
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.
"Bidder request should contain outputModule from param"
| assert !response.ext?.errors | ||
| assert !response.ext?.warnings | ||
|
|
||
| and: "Bidder request should contain gpc from param" |
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.
"Bidder request should contain dnt from param"
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.
Check others for typos
934cb46 to
5056fdd
Compare
🔧 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