-
Notifications
You must be signed in to change notification settings - Fork 224
Tests: Increase coverage for floors logs tests #3945
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
Conversation
| id = UUID.randomUUID() | ||
| impid = imp.id | ||
| price = PBSUtils.getRandomPrice() | ||
| price = imp.bidFloor != null ? PBSUtils.getRandomPrice(imp.bidFloor) : PBSUtils.getRandomPrice() |
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 like we should apply imp.bidFloor != null ? imp.bidFloor : PBSUtils.getRandomPrice()
| "request ${bidRequest.id}, reason: ${URL_EMPTY_WARNING_MESSAGE("$BASIC_FETCH_URL$bidRequest.accountId", message)}" | ||
| } | ||
| // Required: Sync no longer logs "in progress"—only "none" or "error" statuses are recorded | ||
| protected static final String FETCHING_DISABLED_ERROR = 'Fetching is disabled' |
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 removethe comment
- Please use double quotes, not a single "Fetching is disabled"
| } | ||
|
|
||
| def "PBS shouldn't emit error in log and response when floors is not in request and floors fetching disabled for account"() { | ||
| given: "Account with disabled fetching" |
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.
Empty space
| and: "Flush metrics" | ||
| flushMetrics(floorsPbsService) |
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.
Move this block before where:
It isn't mandatory, it's rather a code style for us.
Also, it can be applied to other
| bidRequest << [BidRequest.getDefaultBidRequest(), getBidRequestWithFloors().tap { it.ext.prebid.floors = null }] | ||
| } | ||
|
|
||
| def "PBS should emit error in log and response when data is invalid and floors fetching disabled for account and enabled for 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.
"PBS should emit error in log and response when floor data is empty and floors fetching disabled for account and enabled for request"
| static BigDecimal getRandomPrice(BigDecimal min, BigDecimal max = 10, int scale = 3) { | ||
| getRandomDecimal(min, max).setScale(scale, HALF_UP) | ||
| } |
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.
Don't see a point to have it
| def priceFloors = new AccountPriceFloorsConfig(enabled: true, fetch: new PriceFloorsFetch(enabled: false)) | ||
| def accountAuctionConfig = new AccountAuctionConfig(priceFloors: priceFloors) | ||
| def accountConfig = new AccountConfig(auction: accountAuctionConfig) | ||
| def account = new Account(uuid: bidRequest.accountId, config: accountConfig) | ||
| accountDao.save(account) |
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.
Here you can use getAccountWithEnabledFetch method and just with
.tap{
...priceFloors.enabled = false
}
| assert metrics[ALERT_GENERAL] == 1 | ||
| } | ||
|
|
||
| def "PBS shouldn't emit error in log and response when data is invalid and floors fetching disabled for account and #requestFloors for 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.
same here:
"PBS shouldn't emit error in log and response when floor data is empty and floors fetching disabled for account and #requestFloors for request"
| assert metrics[ALERT_GENERAL] == 1 | ||
| } | ||
|
|
||
| def "PBS shouldn't emit error or warning when floors for account configured correctly"() { |
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.
?
🔧 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