-
Notifications
You must be signed in to change notification settings - Fork 224
Test: Rule Engine #4103
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
Test: Rule Engine #4103
Conversation
…ring result function.
# Conflicts: # extra/modules/rule-engine/src/main/java/org/prebid/server/hooks/modules/rule/engine/core/request/result/functions/filter/FilterBiddersFunction.java
osulzhenko
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.
pls split spec into:
- base (abstract)
- general
- aliases (optional)
- validation
- analytics
also add metrics verification:
success.no-invocation
success.noop
call
success.update
pom.xml
Outdated
| <configuration> | ||
| <systemPropertyVariables> | ||
| <launchContainers>false</launchContainers> | ||
| <launchContainers>true</launchContainers> |
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.
roll back this change
| @JsonProperty("seatnonbid") | ||
| BidRejectionReason seatNonBid = BidRejectionReason.REQUEST_BLOCKED_OPTIMIZED; | ||
|
|
||
| @JsonProperty("ifSyncedId") |
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 shouldn't change java code
| import static org.prebid.server.functional.model.config.Stage.PROCESSED_AUCTION_REQUEST | ||
| import static org.prebid.server.functional.util.PBSUtils.randomString | ||
|
|
||
| class RuleSets { |
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.
RuleSet?
| List<Object> datacenters | ||
| List<Object> sources | ||
| List<Object> sids | ||
| Object pct |
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.
percent?
src/test/groovy/org/prebid/server/functional/model/config/RuleEngineFunctionArgs.groovy
Show resolved
Hide resolved
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| where: | ||
| requestedUfpUser << [new User(data: null), new User(data: [null]), //todo empty |
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.
pls remove all todo
| getDefaultBidRequestWithMultiplyBidders().tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| site.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK | ||
| site.ext = new SiteExt(data: null) | ||
| }, | ||
| getDefaultBidRequestWithMultiplyBidders().tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| site.ext = new SiteExt(data: null) | ||
| }, | ||
| getDefaultBidRequestWithMultiplyBidders(APP).tap { | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it) | ||
| app.content = new Content(data: [null]) //todo PLEASE TAKE A LOOK | ||
| app.ext = new AppExt(data: null) | ||
| }, |
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.
pls remove all todos
| it.ruleSets[0].modelGroups[0].schema = [new RuleEngineModelSchema(function: GPP_SID_AVAILABLE)] | ||
| } | ||
|
|
||
| and: "Account with disabled or without rules engine" |
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.
and: "Account with rules engine". Same for others
| where: | ||
| distributionChannel | bidRequestClosure | ||
| SITE | { String domain -> | ||
| BidRequest.getDefaultBidRequest(distributionChannel).tap { |
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.
mb getDefaultBidRequestWithMultiplyBidders?
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| and: "Logs should contain error" | ||
| def logs = pbsServiceWithRulesEngineModule.getLogsByTime(startTime) |
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.
getLogsByText?
| PaaFormat paaFormat | ||
| @JsonProperty("alternatebiddercodes") | ||
| AlternateBidderCodes alternateBidderCodes | ||
| Map kvps |
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.
@JsonProperty("kvps")
Map<String, String> keyValuePairs
| "adapters.${AMX}.endpoint": "$networkServiceContainer.rootUri/auction".toString()] | ||
| protected static final Map<String, String> OPENX_ALIAS_CONFIG = ["adapters.${OPENX}.aliases.${OPENX_ALIAS}.enabled" : "true", | ||
| "adapters.${OPENX}.aliases.${OPENX_ALIAS}.endpoint": "$networkServiceContainer.rootUri/auction".toString()] | ||
| protected static final String PB_RULE_ENGINE_MODULE_NAME_CODE = "pb-rule-engine" |
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.
replase with ModuleName.PB_RULE_ENGINE
| protected final static String CALL_METRIC = "modules.module.%s.stage.%s.hook.%s.call" | ||
| protected final static String FAILER_METRIC = "modules.module.%s.stage.%s.hook.%s.failure" | ||
| protected final static String NOOP_METRIC = "modules.module.%s.stage.%s.hook.%s.success.noop" | ||
| protected final static String UPDATE_METRIC = "modules.module.%s.stage.%s.hook.%s.success.update" |
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 always PB_RULE_ENGINE module for this spec
| Stage stage | ||
| String name | ||
| String version | ||
| List<RulesEngineModelGroups> modelGroups |
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.
RulesEngineModelGroup
| updateBidRequestWithGeoCountry(it) | ||
| updateBidRequestWithTraceVerboseAndReturnAllBidStatus(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.
you have 125 uses of getDefaultBidRequestWithMultiplyBidders, 36 uses of updateBidRequestWithGeoCountry & 124 - updateBidRequestWithTraceVerboseAndReturnAllBidStatus. So why not combine them?
| AD_UNIT_CODE_IN("adUnitCodeIn"), | ||
| DEVICE_TYPE("deviceType"), | ||
| DEVICE_TYPE_IN("deviceTypeIn"), | ||
| BID_PRICE("bidPrice") |
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.
BID_PRICE not used
| 1 | TRUE as String | ||
| 0 | FALSE as String |
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.
1 | 'TRUE'
0 | 'FALSE'
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| percent << [100, PBSUtils.getRandomNumber(100)] |
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.
Is 100% really that special? and why [100; 0x7fffffff)?
| "Field 'domains' is required and has to be an array of strings") | ||
|
|
||
| where: | ||
| distributionChannel | bidRequestClosure |
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.
can be simplified:
bidRequestClosure << [ getDefaultBidRequestWithMultiplyBidders(SITE).tap {
it.site.publisher = new Publisher(id: PBSUtils.randomString, domain: PBSUtils.randomString)
updateBidRequestWithTraceVerboseAndReturnAllBidStatus(it)
} ]
same for others
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, why not make a methods that sets Publisher or domain based on distributionChannel? Like setAccountId. Then you will only need distributionChannel in where part
| def "PBS should exclude bidder when adUnitCodeIn match with condition"() { | ||
| given: "Default bid request with multiply bidders" | ||
| def randomString = PBSUtils.randomString | ||
| def bidRequest = bidRequestClosure(randomString) |
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.
m.b. generate bidRequest before Closure and only then apply change?
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?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)
getImpUnitCode(Imp imp, ImpUnitCode) {
switch ()
}
enum ImpUnitCode {
TAG_ID
GRID
DATA
STORED_REQUEST
}
# Conflicts: # src/test/groovy/org/prebid/server/functional/model/request/auction/Prebid.groovy
| protected static Map<String, String> getRequestCorrectionSettings(Endpoint endpoint = OPENRTB2_AUCTION, Stage stage = PROCESSED_AUCTION_REQUEST) { | ||
| ["hooks.${PB_REQUEST_CORRECTION.code}.enabled": "true", | ||
| "hooks.host-execution-plan" : encode(ExecutionPlan.getSingleEndpointExecutionPlan(endpoint, PB_REQUEST_CORRECTION, [stage]))] |
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.
pls restore config
| private static void updateImpWithOpenXAndAmxAndOpenXAliasBidder(Bidder bidder) { | ||
| bidder.tap { | ||
| openx = Openx.defaultOpenx | ||
| amx = new Amx() | ||
| openxAlias = Openx.defaultOpenx | ||
| } | ||
| } | ||
|
|
||
| private static void getAliasAndAmxAndOpenXAndWithoutGenericBidder(Bidder bidder) { | ||
| bidder.tap { | ||
| alias = new Generic() | ||
| generic = null | ||
| amx = new Amx() | ||
| openx = Openx.defaultOpenx | ||
| } | ||
| } | ||
| } |
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.
m.b.? And make it protected on RuleEngineBaseSpec for getDefaultBidRequestWithMultiplyBidders
private static void updateBidderImp(Imp imp, List<BidderName> bidders = MULTI_BID_ADAPTERS) {
imp.ext.prebid.bidder.tap {
openx = bidders.contains(OPENX) ? Openx.defaultOpenx : null
openxAlias = bidders.contains(OPENX_ALIAS) ? Openx.defaultOpenx : null
amx = bidders.contains(AMX) ? new Amx() : null
generic = bidders.contains(GENERIC) ? new Generic() : null
alias = bidders.contains(ALIAS) ? new Generic() : null
}
}
This also allow u remove magic numbers
bidResponse.ext.seatnonbid.size() == 3/2
| AD_UNIT_CODE("adUnitCode"), | ||
| AD_UNIT_CODE_IN("adUnitCodeIn"), | ||
| DEVICE_TYPE("deviceType"), | ||
| DEVICE_TYPE_IN("deviceTypeIn"), |
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 bidPrice?
Allows response rules to be set up to trigger on bid response prices. e.g. bidPrice["gt", 5.00, "EUR"] would return true if the bid response is greater than 5 EUR.
Left as an example of possible future functions.
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.
BidPrice will be later, currently it's in low priority
| Stage stage | ||
| String name | ||
| String version | ||
| List<RulesEngineModelGroup> modelGroups |
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.
timestamp?
ruleSets[].timestamp - an optional string indicating when this ruleset was last updated. Again - for troubleshooting. Format is ISO 8601 format.
|
|
||
| static RulesEngineModelGroup createRulesModuleGroup() { | ||
| new RulesEngineModelGroup().tap { | ||
| it.weight = PBSUtils.getRandomNumber(0, 100) |
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.
PBSUtils.getRandomNumber(1, 100)
Could be flaky:
Weight must be greater than zero
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| distributionChannel << [SITE, APP, DOOH] |
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 other
DistributionChannel.values()
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
|
|
||
| where: | ||
| bidRequestClosure << [ |
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.
BidRequest createBidRequest(DistributionChannel type, String domain, boolean usePublisher = true) {
def request = getDefaultBidRequestWithMultiplyBidders(type)
switch (type) {
case SITE:
if (usePublisher) request.site.publisher.domain = domain
else request.site.domain = domain
break
case APP:
if (usePublisher) request.app.publisher.domain = domain
else request.app.domain = domain
break
case DOOH:
if (usePublisher) request.dooh.publisher.domain = domain
else request.dooh.domain = domain
break
}
request
}
type | usePublisher
SITE | true
SITE | false
APP | true
APP | false
| assert !getAnalyticResults(bidResponse) | ||
| } | ||
|
|
||
| def "PBS should exclude bidder when adUnitCode match with condition"() { |
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 priority test:
Return the first of: imp.ext.gpid, imp.tagid, imp.ext.data.pbadslot, imp.ext.prebid.storedrequest.id
In validation mode, reject args
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.
1.Ok
2.We have test for this condition : PBS should reject processing rule engine when #function schema function contain args
| def "PBS should exclude bidder when adUnitCodeIn match with condition"() { | ||
| given: "Default bid request with multiply bidders" | ||
| def randomString = PBSUtils.randomString | ||
| def bidRequest = bidRequestClosure(randomString) |
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?
def randomString = PBSUtils.randomString
def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap {
imp[0].tagId = PBSUtils.randomString
imp[0].ext.gpid = PBSUtils.randomString
imp[0].ext.data = new ImpExtContextData(pbAdSlot: PBSUtils.randomString)
imp[0].ext.prebid.storedRequest = new PrebidStoredRequest(id: PBSUtils.randomString)
}
def randomString = getImpUnitCode(bidRequest.imp[0], code)
getImpUnitCode(Imp imp, ImpUnitCode) {
switch ()
}
enum ImpUnitCode {
TAG_ID
GRID
DATA
STORED_REQUEST
}
| assert seatNonBid.nonBid[0].statusCode == REQUEST_BIDDER_REMOVED_BY_RULE_ENGINE_MODULE | ||
| } | ||
|
|
||
| def "PBS shouldn't exclude bidder when percent not match with condition"() { |
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.
when percent less than zero
| AD_UNIT_CODE("adUnitCode",null), | ||
| AD_UNIT_CODE_IN("adUnitCodeIn","codes"), | ||
| DEVICE_TYPE("deviceType",null), | ||
| DEVICE_TYPE_IN("deviceTypeIn","types"), |
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.
formating
|
|
||
| import org.prebid.server.functional.model.request.auction.Imp | ||
|
|
||
| import static org.prebid.server.functional.model.ModuleName.* |
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.
replace wildcard
| impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue | ||
| impResult.values.resultFunction == groups.rules.first.results.first.function.value | ||
| impResult.values.conditionFired == groups.rules.first.conditions.first | ||
| impResult.values.biddersRemoved.sort() == [OPENX, AMX, GENERIC].sort() |
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.
can be def value
[OPENX, AMX, GENERIC].sort()
| impResult.values.analyticsValue == groups.rules.first.results.first.args.analyticsValue | ||
| impResult.values.resultFunction == groups.rules.first.results.first.function.value | ||
| impResult.values.conditionFired == groups.rules.first.conditions.first | ||
| impResult.values.biddersRemoved.sort() == [OPENX, GENERIC, AMX].sort() |
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
| } | ||
|
|
||
| protected static String getImpAdUnitCode(Imp imp) { | ||
| if (imp.ext.gpid) { |
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 ?. for null safe
| given: "Bid request with multiply imps bidders" | ||
| def bidRequest = getDefaultBidRequestWithMultiplyBidders().tap { | ||
| it.imp.add(Imp.defaultImpression) | ||
| it.imp[1].ext.prebid.bidder.tap { |
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.
updateBidderImp?
same for others
|
|
||
|
|
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
| assert !getAnalyticResults(bidResponse) | ||
|
|
||
| where: | ||
| gdpr | condition |
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.
not consistent with previous. M.b. in lower and upper cases?
| and: "Analytics result shouldn't contain info about module exclude" | ||
| assert !getAnalyticResults(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.
empty space
| where: | ||
| gpid | tagId | pbAdSlot | prebidStoredRequest | ||
| PBSUtils.getRandomString() | null | null | null | ||
| null | PBSUtils.getRandomString() | null | null | ||
| null | null | PBSUtils.getRandomString() | null | ||
| null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString()) | ||
| null | null | PBSUtils.getRandomString() | null | ||
| null | PBSUtils.getRandomString() | null | null | ||
| PBSUtils.getRandomString() | null | null | null |
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.
where:
gpid | tagId | pbAdSlot | prebidStoredRequest
PBSUtils.getRandomString() | null | null | null
null | PBSUtils.getRandomString() | null | null
null | null | PBSUtils.getRandomString() | null
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
PBSUtils.getRandomString() | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | PBSUtils.getRandomString() | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | PBSUtils.getRandomString() | new PrebidStoredRequest(id: PBSUtils.getRandomString())
null | null | null | new PrebidStoredRequest(id: PBSUtils.getRandomString())
🔧 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