Add ImpIds in RequestData for associated Impressions #3364
Conversation
There was a problem hiding this comment.
The approach here looks good. There's still a major effort left though to update the tests which involves explicitly defining what the expected RequestData.ImpIDs are in every adapter's JSON tests via httpCalls[i].expectedRequests.
The team discussed a few possible test solutions and figured we should provide the test framework update we're looking for so that we can be sure we avoid rework.
Please add the following field to httpRequest here (https://github.com/prebid/prebid-server/blob/master/adapters/adapterstest/test_json.go#L203):
ImpIDs []string `json:"impIDs"`
If you could then add the following just before the last return statement here (https://github.com/prebid/prebid-server/blob/master/adapters/adapterstest/test_json.go#L321) and update all of the JSON tests accordingly that would be great:
if len(expected.ImpIDs) < 1 {
return fmt.Errorf(`expected.ImpIDs must contain at least one imp ID`)
}
if !reflect.DeepEqual(expected.ImpIDs, actual.ImpIDs) {
return fmt.Errorf(`%s actual.ImpIDs "%q" do not match expected "%q"`, description, actual.ImpIDs, expected.ImpIDs)
}
|
Hello @bsardo, |
There was a problem hiding this comment.
@pm-saurabh-narkhede Your approach looks good. I think you can continue with this for the remaining adapters.
Hi @pm-saurabh-narkhede this commit looks good. Is this ready for review? At first glance it looks like you updated all of the applicable JSON tests. |
Hi @bsardo , |
|
@pm-saurabh-narkhede no rush, I just want to make sure you saw my couple of other comments requesting changes. Once those are made I will re-review. |
@bsardo I have made the changes. You can review them. |
|
blocks #2852 |
| @@ -73,7 +73,8 @@ | |||
| "X-Forwarded-For": [ | |||
| "0.0.0.0" | |||
| ] | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
It is a shame that adgeneration does not have any multi-imp tests. But that is beyond the scope of this PR. We will have to try to be more picky about that in adapter reviews.
There was a problem hiding this comment.
Agree with you @hhhjort .
Thanks for noticing it out.
|
Hi @SyntaxNode , @hhhjort , @bsardo , |
Implements #2973