Skip to content

Conversation

@osulzhenko
Copy link
Collaborator

@osulzhenko osulzhenko commented Feb 3, 2025

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ 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?

@osulzhenko osulzhenko added the tests Functional or other tests label Feb 3, 2025
@osulzhenko osulzhenko requested a review from marki1an February 3, 2025 16:59
@osulzhenko osulzhenko self-assigned this Feb 3, 2025
def infrastructure = bidResponse.ext.debug.trace.activityInfrastructure
def ruleConfigurations = findProcessingRule(infrastructure, FETCH_BIDS).ruleConfiguration.and
assert ruleConfigurations.size() == 1
assert ruleConfigurations.first.and.every { it != null }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to assert for the and field?

Comment on lines 503 to 504
where:
defaultAction << [false, true]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we can emit this where block

Comment on lines 14 to 16
String privacyModule
String skipped
String result
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to replace on:

PrivacyModule privacyModule
Boolean skipped
Result(Need to create Enum also need to replace in ActivityInfrastructure) result

it.geo = [CAN.withState(ARIZONA)]
}
def activityRule = ActivityRule.getDefaultActivityRule(condition).tap {
it.privacyRegulation = [IAB_US_GENERAL]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's not only for iab.usgeneral.
Can we add tests for example IAB_ALL or just ALL?

Comment on lines 410 to 413
device = new Device(geo: new Geo(country: USA, region: ALABAMA.abbreviation))
regs.ext = new RegsExt(gpc: PBSUtils.randomString)
regs.gppSid = [US_CA_V1.intValue]
regs.gpp = new UsNatV1Consent.Builder().setGpc(true).build()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those fields are unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relay on other rest tests

Comment on lines 420 to 421
it.gpc = gpc
it.geo = [CAN.withState(ARIZONA)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those fields are unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relay on other rest tests

}

and: "Set up activities"
def gpc = PBSUtils.randomString
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also unnecessary

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relay on other rest tests


and: "Should contain information that module was skipped"
verifyAll(ruleConfigurations.first) {
it.privacyModule == 'iab.usgeneral'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it.privacyModule == PrivacyModule.IAB_US_GENERAL

defaultAction << [false, true]
}

def "PBS auction should log consistently for each activity about skips modules in response"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems like flaky tests

@osulzhenko osulzhenko requested a review from marki1an February 5, 2025 22:30
@osulzhenko osulzhenko force-pushed the functional-tests/privacy-modules-skip-rate branch from 7ae0cc5 to 517ca94 Compare February 5, 2025 22:37
assert genericBidderRequest.user.eids[0].source == bidRequest.user.eids[0].source

and: "Bid response should not contain verbose info in debug"
def infrastructure = bidResponse.ext.debug.trace.activityInfrastructure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Bid response should contain verbose info in debug"

}
}

def "PBS auction shouldn't emit errors or warnings when skip rate is out of borders"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about checking for bidRequest.user.eids[0].source?

@osulzhenko osulzhenko requested a review from marki1an February 6, 2025 09:11
@osulzhenko osulzhenko merged commit f3cd04d into privcay-modules-skip-rate Feb 6, 2025
1 check passed
@osulzhenko osulzhenko deleted the functional-tests/privacy-modules-skip-rate branch February 6, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Functional or other tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants