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.

Sign up for GitHub

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

Jump to bottom

Test: default bids cache ttl #3558

Open

marki1an wants to merge 3 commits into default-bids-cache-ttl from tests/default-bids-cache-ttl

Collaborator

marki1an commented Nov 19, 2024 •

edited

Loading

🔧 Type of changes

✨ What's the context?

What's the context for the changes? Are there any

🧠 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 hosts
geographic host parameters are NOT required
NO 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?


          Add tests for default bids cache ttl

e6babcd

marki1an added the tests label

marki1an self-assigned this

marki1an changed the title ~~Add tests for default bids cache ttl~~ Tests: default bids cache ttl

marki1an changed the title ~~Tests: default bids cache ttl~~ Test: default bids cache ttl

osulzhenko requested changes

View reviewed changes

Collaborator

osulzhenko left a comment

Also, please format the cases by similarity. Group the tests for priority in descending order, etc.

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 20 to 41

+                  private static def HOST_BANNER_TTL = PBSUtils.randomNumber
+                  private static def HOST_VIDEO_TTL = PBSUtils.randomNumber
+                  private static def DEFAULT_TTL_SECONDS_BANNER = PBSUtils.randomNumber
+                  private static def DEFAULT_TTL_SECONDS_VIDEO = PBSUtils.randomNumber
+                  private static def DEFAULT_TTL_SECONDS_AUDIO = PBSUtils.randomNumber
+                  private static def DEFAULT_TTL_SECONDS_NATIVE = PBSUtils.randomNumber
+                  private static final Map<String, String> CONFIG_CACHE_TTL_SECONDS = ["cache.banner-ttl-seconds": HOST_BANNER_TTL as String,
+                                                                                       "cache.video-ttl-seconds" : HOST_VIDEO_TTL as String]
+                  private static final Map<String, String> CONFIG_CACHE_TTL_DEFAULT_SECONDS = ["cache.default-ttl-seconds.banner": DEFAULT_TTL_SECONDS_BANNER as String,
+                                                                                               "cache.default-ttl-seconds.video" : DEFAULT_TTL_SECONDS_VIDEO as String,
+                                                                                               "cache.default-ttl-seconds.native": DEFAULT_TTL_SECONDS_NATIVE as String,
+                                                                                               "cache.default-ttl-seconds.audio" : DEFAULT_TTL_SECONDS_AUDIO as String]
+                  private static final Map<String, String> EMPTY_DEFAULT_TTL_SECOND_CONFIG = ["cache.default-ttl-seconds.banner": "",
+                                                                                              "cache.default-ttl-seconds.video" : "",
+                                                                                              "cache.default-ttl-seconds.native": "",
+                                                                                              "cache.default-ttl-seconds.audio" : ""]
+                  private static final Map<String, String> EMPTY_CACHE_TTL_SECOND = ["cache.banner-ttl-seconds": "",
+                                                                                     "cache.video-ttl-seconds" : ""]
+                  private static def pbsWithOnlyCacheSecondsTtlService = pbsServiceFactory.getService(CONFIG_CACHE_TTL_SECONDS + EMPTY_DEFAULT_TTL_SECOND_CONFIG)
+                  private static def pbsWithoutCacheTtlService = pbsServiceFactory.getService(EMPTY_DEFAULT_TTL_SECOND_CONFIG + EMPTY_CACHE_TTL_SECOND)
+                  private static def pbsWithCacheSecondsAndDefaultTtlSecondsService = pbsServiceFactory.getService(CONFIG_CACHE_TTL_SECONDS + CONFIG_CACHE_TTL_DEFAULT_SECONDS)
+                  private static def pbsWithOnlyDefaultCacheSecondsTtlService = pbsServiceFactory.getService(EMPTY_CACHE_TTL_SECOND + CONFIG_CACHE_TTL_DEFAULT_SECONDS)

Collaborator

osulzhenko Nov 21, 2024

I suggest improving naming of variables and structures to make them more readable and consistent. The primary idea is to highlight the unique type (BANNER, VIDEO, etc.) at beginning of each name:

    private static final def BANNER_TTL_HOST_CACHE = PBSUtils.randomNumber
    private static final def VIDEO_TTL_HOST_CACHE = PBSUtils.randomNumber
    private static final def BANNER_TTL_DEFAULT_CACHE = PBSUtils.randomNumber
    private static final def VIDEO_TTL_DEFAULT_CACHE = PBSUtils.randomNumber
    private static final def AUDIO_TTL_DEFAULT_CACHE = PBSUtils.randomNumber
    private static final def NATIVE_TTL_DEFAULT_CACHE = PBSUtils.randomNumber
    private static final Map<String, String> CACHE_TTL_HOST_CONFIG = ["cache.banner-ttl-seconds": BANNER_TTL_HOST_CACHE as String,
                                                                      "cache.video-ttl-seconds" : VIDEO_TTL_HOST_CACHE as String]
    private static final Map<String, String> DEFAULT_CACHE_TTL_CONFIG = ["cache.default-ttl-seconds.banner": BANNER_TTL_DEFAULT_CACHE as String,
                                                                         "cache.default-ttl-seconds.video" : VIDEO_TTL_DEFAULT_CACHE as String,
                                                                         "cache.default-ttl-seconds.native": NATIVE_TTL_DEFAULT_CACHE as String,
                                                                         "cache.default-ttl-seconds.audio" : AUDIO_TTL_DEFAULT_CACHE as String]
    private static final Map<String, String> EMPTY_CACHE_TTL_CONFIG = ["cache.default-ttl-seconds.banner": "",
                                                                       "cache.default-ttl-seconds.video" : "",
                                                                       "cache.default-ttl-seconds.native": "",
                                                                       "cache.default-ttl-seconds.audio" : ""]
    private static final Map<String, String> EMPTY_CACHE_TTL_HOST_CONFIG = ["cache.banner-ttl-seconds": "",
                                                                            "cache.video-ttl-seconds" : ""]
    private static def pbsOnlyHostCacheTtlService = pbsServiceFactory.getService(CACHE_TTL_HOST_CONFIG + EMPTY_CACHE_TTL_CONFIG)
    private static def pbsEmptyTtlService = pbsServiceFactory.getService(EMPTY_CACHE_TTL_CONFIG + EMPTY_CACHE_TTL_HOST_CONFIG)
    private static def pbsHostAndDefaultCacheTtlService = pbsServiceFactory.getService(CACHE_TTL_HOST_CONFIG + DEFAULT_CACHE_TTL_CONFIG)
    private static def pbsOnlyDefaultCacheTtlService = pbsServiceFactory.getService(EMPTY_CACHE_TTL_HOST_CONFIG + DEFAULT_CACHE_TTL_CONFIG)

Collaborator

osulzhenko Nov 21, 2024

pbsWithOnlyDefaultCacheSecondsTtlService has only one use, so please include it as a test step.

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

                       then: "Bid response should contain exp data"
                       assert response.seatbid.bid.first.exp == [accountCacheTtl]
                   }
+                  def "PBS auction should resolve bid.exp for #mediaType when imp contain #mediaType and bid responded with bid.exp"() {

Collaborator

osulzhenko Nov 21, 2024

PBS auction should prioritize bid.exp from the response over all other fields from the request and account config

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                      mediaType << [BANNER, VIDEO, NATIVE, AUDIO]
+                  }
+                  def "PBS auction shouldn't resolve bid.exp for #mediaType when imp contain #mediaType and bid without exp"() {

Collaborator

osulzhenko Nov 21, 2024

PBS auction shouldn't resolve bid.exp for #mediaType when the response, request, and account config don't include such data

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                      mediaType << [BANNER, VIDEO, NATIVE, AUDIO]
+                  }
+                  def "PBS auction should resolve bid.exp for #mediaType when imp contain #mediaType and imp.exp"() {

Collaborator

osulzhenko Nov 21, 2024

PBS auction should prioritize imp.exp and resolve bid.exp for #mediaType when request and account config include multiple exp sources

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 399 to 419

+                  def "PBS auction shouldn't resolve bid.exp for #mediaType when imp contain #mediaType and doesn't imp.exp"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          imp[0] = Imp.getDefaultImpression(mediaType).tap {
+                              exp = null
+                          }
+                      }
+                      and: "Default bid response"
+                      def bidResponse = BidResponse.getDefaultBidResponse(bidRequest)
+                      bidder.setResponse(bidRequest.id, bidResponse)
+                      when: "PBS processes auction request"
+                      def response = pbsWithoutCacheTtlService.sendAuctionRequest(bidRequest)
+                      then: "Bid response shouldn't contain exp data"
+                      assert !response.seatbid.first.bid.first.exp
+                      where:
+                      mediaType << [BANNER, VIDEO, NATIVE, AUDIO]
+                  }

Collaborator

osulzhenko Nov 21, 2024

I assume this test is similar with "PBS auction shouldn't resolve bid.exp for #mediaType when imp contain #mediaType and bid without exp" and duplicate it functionality

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 595 to 636

+                  def "PBS auction should resolve bid.exp when account.auction.banner-cache-ttl and banner bid specified"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          enableCache()
+                          imp[0] = Imp.getDefaultImpression(BANNER)
+                      }
+                      and: "Account in the DB"
+                      def accountCacheTtl = PBSUtils.randomNumber
+                      def auctionConfig = new AccountAuctionConfig(bannerCacheTtl: accountCacheTtl)
+                      def account = new Account(uuid: bidRequest.accountId, config: new AccountConfig(auction: auctionConfig))
+                      accountDao.save(account)
+                      when: "PBS processes auction request"
+                      def response = pbsWithoutCacheTtlService.sendAuctionRequest(bidRequest)
+                      then: "Bid response should contain exp data"
+                      assert response.seatbid.first.bid.first.exp == accountCacheTtl
+                  }
+                  def "PBS auction should resolve bid.exp when account.auction.video-cache-ttl and video bid specified"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          imp[0] = Imp.getDefaultImpression(VIDEO)
+                      }
+                      and: "Default bid response"
+                      def bidResponse = BidResponse.getDefaultBidResponse(bidRequest)
+                      bidder.setResponse(bidRequest.id, bidResponse)
+                      and: "Account in the DB"
+                      def accountCacheTtl = PBSUtils.randomNumber
+                      def auctionConfig = new AccountAuctionConfig(videoCacheTtl: accountCacheTtl)
+                      def account = new Account(uuid: bidRequest.accountId, config: new AccountConfig(auction: auctionConfig))
+                      accountDao.save(account)
+                      when: "PBS processes auction request"
+                      def response = pbsWithoutCacheTtlService.sendAuctionRequest(bidRequest)
+                      then: "Bid response should contain exp data"
+                      assert response.seatbid.first.bid.first.exp == accountCacheTtl
+                  }

Collaborator

osulzhenko Nov 21, 2024

could be combined

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 638 to 667

+                  def "PBS auction should resolve bid.exp when cache.banner-ttl-seconds config specified"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          imp[0] = Imp.getDefaultImpression(BANNER)
+                          enableCache()
+                      }
+                      when: "PBS processes auction request"
+                      def response = pbsWithOnlyCacheSecondsTtlService.sendAuctionRequest(bidRequest)
+                      then: "Bid response should contain exp data"
+                      assert response.seatbid.first.bid.first.exp == HOST_BANNER_TTL
+                  }
+                  def "PBS auction should resolve bid.exp when cache.video-ttl-seconds config specified"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          imp[0] = Imp.getDefaultImpression(VIDEO)
+                      }
+                      and: "Set bidder response"
+                      def bidResponse = BidResponse.getDefaultBidResponse(bidRequest)
+                      bidder.setResponse(bidRequest.id, bidResponse)
+                      when: "PBS processes auction request"
+                      def response = pbsWithOnlyCacheSecondsTtlService.sendAuctionRequest(bidRequest)
+                      then: "Bid response should contain exp data"
+                      assert response.seatbid.first.bid.first.exp == HOST_VIDEO_TTL
+                  }

Collaborator

osulzhenko Nov 21, 2024

also could be combined

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                      assert response.seatbid.first.bid.first.exp == HOST_VIDEO_TTL
+                  }
+                  def "PBS auction shouldn't resolve bid.exp when cache.{banner/video}-ttl-seconds specified and media type is #mediaType"() {

Collaborator

osulzhenko Nov 21, 2024

PBS auction shouldn't resolve bid.exp when cache ttl-seconds is specified for #mediaType mediaType request

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 671 to 673

+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          imp[0] = Imp.getDefaultImpression(mediaType)
+                      }

Collaborator

osulzhenko Nov 21, 2024

please specify cache ttl-seconds

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                      mediaType << [NATIVE, AUDIO]
+                  }
+                  def "PBS auction should resolve bid.exp when cache.default-ttl-seconds.{banner,video,audio,native} last place where present value for bid.exp"() {

Collaborator

osulzhenko Nov 21, 2024

PBS auction should resolve bid.exp when cache.default-ttl-seconds.{banner,video,audio,native} is specified and no higher-priority fields are present


          Update after review

6110c5e

osulzhenko requested changes

View reviewed changes

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 539 to 553

+                      mediaType | auctionConfig
+                      VIDEO     | new AccountAuctionConfig()
+                      VIDEO     | new AccountAuctionConfig(bannerCacheTtl: null)
+                      VIDEO     | new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: null)
+                      BANNER    | new AccountAuctionConfig()
+                      BANNER    | new AccountAuctionConfig(videoCacheTtl: null)
+                      BANNER    | new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: null)
+                      NATIVE    | new AccountAuctionConfig()
+                      NATIVE    | new AccountAuctionConfig(bannerCacheTtl: null)
+                      NATIVE    | new AccountAuctionConfig(videoCacheTtl: null)
+                      NATIVE    | new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: null)
+                      AUDIO     | new AccountAuctionConfig()
+                      AUDIO     | new AccountAuctionConfig(bannerCacheTtl: null)
+                      AUDIO     | new AccountAuctionConfig(videoCacheTtl: null)
+                      AUDIO     | new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: null)

Collaborator

osulzhenko Nov 26, 2024

From my perspective of view options:
new AccountAuctionConfig(), new AccountAuctionConfig(bannerCacheTtl: null), new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: null)
are identical. So we can simplify this code:

where:
mediaType << [VIDEO, BANNER, NATIVE, AUDIO]

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                  def "PBS auction shouldn't resolve bid.exp when account config and request imp type match but account config for cache-ttl is not specified"() {
+                      given: "Default bid request"
+                      def bidRequest = BidRequest.getDefaultBidRequest()

Collaborator

osulzhenko Nov 26, 2024

    def bidRequest = BidRequest.getDefaultBidRequest().tap {
        enableCache()
        imp[0] = Imp.getDefaultImpression(mediaType)
    }

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy

+                      given: "Default bid request"
+                      def randomExp = PBSUtils.randomNumber
+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          enableCache()

Collaborator

osulzhenko Nov 26, 2024

Do we really need this enableCache() in our tests?

Collaborator Author

marki1an Nov 28, 2024

Can't just delete it.
In order to see cache in our flow we should have those two obj in request

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 506 to 517

+                      VIDEO     | new AccountAuctionConfig(videoCacheTtl: null)
+                      VIDEO     | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber)
+                      VIDEO     | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber, videoCacheTtl: null)
+                      BANNER    | new AccountAuctionConfig(bannerCacheTtl: null)
+                      BANNER    | new AccountAuctionConfig(videoCacheTtl: PBSUtils.randomNumber)
+                      BANNER    | new AccountAuctionConfig(bannerCacheTtl: null, videoCacheTtl: PBSUtils.randomNumber)
+                      NATIVE    | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber, videoCacheTtl: PBSUtils.randomNumber)
+                      NATIVE    | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber)
+                      NATIVE    | new AccountAuctionConfig(videoCacheTtl: PBSUtils.randomNumber)
+                      AUDIO     | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber, videoCacheTtl: PBSUtils.randomNumber)
+                      AUDIO     | new AccountAuctionConfig(bannerCacheTtl: PBSUtils.randomNumber)
+                      AUDIO     | new AccountAuctionConfig(videoCacheTtl: PBSUtils.randomNumber)

Collaborator

osulzhenko Nov 26, 2024

new AccountAuctionConfig(videoCacheTtl: null) and new AccountAuctionConfig(bannerCacheTtl: null) are the same as new AccountAuctionConfig(). And we testing it in PBS auction shouldn't resolve bid.exp when account config and request imp type match but account config for cache-ttl is not specified

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

Comment on lines 634 to 636

+                      def response = pbsServiceFactory
+                              .getService(EMPTY_CACHE_TTL_HOST_CONFIG + DEFAULT_CACHE_TTL_CONFIG)
+                              .sendAuctionRequest(bidRequest)

Collaborator

osulzhenko Nov 26, 2024

Not like this, because you still have an open container in memory, and it’s possible that you will face issues with unexpected closing of the active one.
Please add a configuration step and a cleanup:

cleanup:
    pbsServiceFactory.removeContainer(config)

as example:

PBS should include analytics tag in response when request and account allow client details but default doesn't

src/test/groovy/org/prebid/server/functional/tests/BidExpResponseSpec.groovy Outdated

+                      def bidRequest = BidRequest.getDefaultBidRequest().tap {
+                          enableCache()
+                          imp[0] = Imp.getDefaultImpression(VIDEO)
+                          ext.prebid.cache = new PrebidCache(vastXml: new PrebidCacheSettings(ttlSeconds: randomExp))

Collaborator

osulzhenko Nov 26, 2024

pls add variant where:

vastXml: randomExp,
bids: new PrebidCacheSettings(ttlSeconds: PBSUtils.randomNumber))

just to be sure that it works correctly


          Updated due to provided comments

8fc4323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests