Tests: Record a default core bid ranking by osulzhenko · Pull Request #3833 · prebid/prebid-server-java

osulzhenko · 2025-03-13T18:36:50Z

🔧 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

verify email contact works
NO fully dynamic hostnames
geographic host parameters are NOT required
direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
cover an adapter configuration with an integration test

🧪 Test plan

How do you know the changes are safe to ship to production?

🏎 Quality check

Are your changes following our code style guidelines?
Are there any breaking changes in your code?
Does your test coverage exceed 90%?
Are there any erroneous console logs, debuggers or leftover code in your changes?

marki1an · 2025-04-01T13:18:00Z

+        then: "PBS should rank bid with deal as top priority"
+        def bids = response.seatbid.first.bid
+        assert bids.find(it -> it.id == bidBDeal.id).ext.prebid.rank == 1
+        assert bids.find(it -> it.id == bidBiggerPrice.id).ext.prebid.rank == 2


I hope bid.price (highest first)

Check other occurrences

marki1an · 2025-04-01T13:21:05Z

+        assert bids.find(it -> it.id == bidBiggerPrice.id).ext.prebid.rank == 2
+    }
+
+    def "PBS should ignore bid ranked from original response when auction.ranking default"() {


Here we can add a where block and add a case for default and false, and we should receive the same result

marki1an · 2025-04-01T13:28:39Z

+        and: "Account in the DB"
+        def accountAuctionConfig = new AccountAuctionConfig(ranking: new AuctionRankingConfig(enabled: true))
+        def accountConfig = new AccountConfig(status: ACTIVE, auction: accountAuctionConfig)
+        def account = new Account(uuid: bidRequest.accountId, config: accountConfig)
+        accountDao.save(account)


Can be in a static method like getAccountConfigWithEnabledAuctionRanking()

marki1an · 2025-04-01T13:56:30Z

+
+        and: "Account in the DB"
+        def accountId = PBSUtils.randomNumber.toString()
+


Please remove empty line

marki1an · 2025-04-01T14:01:33Z

@@ -0,0 +1,6 @@
+package org.prebid.server.functional.model.config
+
+class AuctionCacheConfig {


@tostring(includeNames = true, ignoreNulls = true)

marki1an · 2025-04-01T14:01:39Z

@@ -0,0 +1,6 @@
+package org.prebid.server.functional.model.config
+
+class AuctionRankingConfig {


@tostring(includeNames = true, ignoreNulls = true)

…unctional-tests/bid-ranking

…unctional-tests/bid-ranking # Conflicts: # src/test/groovy/org/prebid/server/functional/model/config/AccountAuctionConfig.groovy

AntoxaAntoxic · 2025-06-05T07:53:49Z

+        ]
+    }
+
+    def "PBS shouldn't add bid ranked for multiple media types request when account config for auction.ranking disabled or default"() {


I would leave only a one such test that starts with PBS shouldn't add bid ranked, because ranking depends on multibid config and not vice versa, so those tests look redundant except for the fact they check the bid ranking config

AntoxaAntoxic · 2025-06-05T07:56:51Z

+        def response = defaultPbsService.sendAuctionRequest(bidRequest)
+
+        then: "PBS should rank single bid"
+        assert response.seatbid.first.bid?.ext?.prebid?.rank?.flatten() == [1]


I'd say the bid ranking feature make sense only when there are at least two bids in order to cover correctness of the sorting logic

in the particular test it mentions preferDeals but it can't be covered because only one bid is present

if you want to show that the second bid was removed due to multibid config I still suggest using another valid bid in order to show that the ranking isn't broken

e.g.
multibid1.price = 1
multibid2.price = 2
bid3.price = 0.5

the result is multibid2.rank = 1, bid3.rank = 2 (not 3)

AntoxaAntoxic · 2025-06-05T08:00:36Z

+        assert response.seatbid.first.bid?.ext?.prebid?.rank?.flatten() == [1]
+    }
+
+    def "PBS should add bid ranked and rank by deals for request with multiBid when auction.ranking and preferDeals are enabled"() {


one important thing that is missed in tests is understanding that the sorting and ranking happens within an imp, and I don't see any test mention it while it's a core requirement and I don't see any test covers two bids for different imps that have the same rank

AntoxaAntoxic · 2025-06-05T08:11:12Z

        }
    }

+    static List<Bid> getDefaultMultyTypesBids(Imp imp, @DelegatesTo(Bid) Closure commonInit = null) {


AntoxaAntoxic · 2025-06-05T08:14:06Z

+        assert bids.find(it -> it.id == bidBiggerPrice.id).ext.prebid.rank == 2
+    }
+
+    def "PBS should add bid ranked and rank by deals for multiple media types request when auction.ranking and preferDeals are enabled"() {


I don't understand the difference between this test and PBS should add bid ranked and rank by deals for request with multiBid when auction.ranking and preferDeals are enabled

looks like the set up is pretty much the same, and the multiple media type request doesn't have any impact on the outcome, since the outcome is mocked identically

marki1an · 2025-06-06T11:18:58Z

+        assert bids.find(it -> it.id == bidBiggerPrice.id).ext.prebid.rank == 1
+        assert bids.find(it -> it.id == bidBDeal.id).ext.prebid.rank == 2


Please consider to replace magic 1 and 2 numbers with something meaningful

marki1an · 2025-06-06T11:20:49Z

+            it.price = bidPrice + 1
+            it.ext = new BidExt(prebid: new Prebid(rank: PBSUtils.randomNumber))
+        }
+        def bidBDeal = Bid.getDefaultBid(bidRequest.imp[0]).tap {


Rename with like bidWithDealId

marki1an · 2025-06-06T11:41:49Z

+        def prebidServerService = pbsServiceFactory.getService(pbsConfig)
+
+        and: "Bid request with multiple bidders"
+        def bidRequest = BidRequest.getDefaultVideoRequest().tap {


Why only video request?

marki1an · 2025-06-06T11:43:19Z

+        assert bids.find(it -> it.id == lowerPriceBid.id).ext.prebid.rank == 2
+
+        and: "PBS should contain error for invalid bid"
+        assert response.ext.errors[ErrorType.GENERIC]?.message?.any { it.contains(middlePriceBid.id) }


Can you add proper assets for error with the full sentence

This is not a validation test case. It shouldn’t matter what error is here — main task is to verify that PBS sets ranks correctly

marki1an · 2025-06-06T11:44:04Z

+
+        def middlePriceBid = Bid.getDefaultBid(bidRequest.imp.first).tap {
+            price = bidPrice + 1
+            adm = null


Why was the adm removed?

adm was removed to provoke a bid error; it could be any type of error except for a pricing one.
An error with adm just popped into my mind first, that’s all

Tests: Record a default core bid ranking

9e13960

osulzhenko added the tests Functional or other tests label Mar 13, 2025

osulzhenko requested a review from marki1an March 13, 2025 18:36

osulzhenko self-assigned this Mar 13, 2025

osulzhenko added 2 commits March 21, 2025 10:16

Merge branch 'refs/heads/master' into functional-tests/bid-ranking

2d6ca82

update tests

fab2140

marki1an requested changes Apr 1, 2025

View reviewed changes

osulzhenko added the blocked label Apr 11, 2025

update after review

a699528

osulzhenko requested a review from marki1an May 28, 2025 08:34

osulzhenko removed the blocked label May 28, 2025

osulzhenko changed the base branch from master to bid-ranking June 3, 2025 18:46

osulzhenko added 2 commits June 3, 2025 21:50

Merge remote-tracking branch 'refs/remotes/origin/bid-ranking' into f…

0f43e43

…unctional-tests/bid-ranking

Update functional tests

34428bb

osulzhenko force-pushed the functional-tests/bid-ranking branch from 5035474 to 34428bb Compare June 3, 2025 21:09

osulzhenko added 2 commits June 5, 2025 00:23

Update functional tests

70010ce

Merge remote-tracking branch 'refs/remotes/origin/bid-ranking' into f…

84fa513

…unctional-tests/bid-ranking # Conflicts: # src/test/groovy/org/prebid/server/functional/model/config/AccountAuctionConfig.groovy

AntoxaAntoxic suggested changes Jun 5, 2025

View reviewed changes

update after review

d08b483

osulzhenko requested a review from AntoxaAntoxic June 5, 2025 12:47

osulzhenko and others added 2 commits June 6, 2025 00:32

update after review

308b828

Merge branch 'bid-ranking' into functional-tests/bid-ranking

0efc9fd

AntoxaAntoxic approved these changes Jun 6, 2025

View reviewed changes

marki1an requested changes Jun 6, 2025

View reviewed changes

update after review

7c4d9e1

osulzhenko requested a review from marki1an June 9, 2025 13:54

marki1an approved these changes Jun 10, 2025

View reviewed changes

osulzhenko merged commit 96c0727 into bid-ranking Jun 10, 2025
1 check passed

osulzhenko deleted the functional-tests/bid-ranking branch June 10, 2025 09:41


		and: "Account in the DB"
		def accountId = PBSUtils.randomNumber.toString()

		@@ -0,0 +1,6 @@
		package org.prebid.server.functional.model.config

		class AuctionCacheConfig {

		@@ -0,0 +1,6 @@
		package org.prebid.server.functional.model.config

		class AuctionRankingConfig {

		assert bids.find(it -> it.id == bidBiggerPrice.id).ext.prebid.rank == 1
		assert bids.find(it -> it.id == bidBDeal.id).ext.prebid.rank == 2

Conversation

osulzhenko commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔧 Type of changes

✨ What's the context?

🧠 Rationale behind the change

🔎 New Bid Adapter Checklist

🧪 Test plan

🏎 Quality check

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

osulzhenko commented Mar 13, 2025 •

edited

Loading