Correct MakeRequests() output assertion in adapter JSON tests #2087
Conversation
| } | ||
| expectedHeader, err := json.Marshal(expected.Headers) | ||
| if err != nil { | ||
| return fmt.Errorf(`%s expected.Headers could not be marshalled. Error: %s"`, description, err.Error()) |
There was a problem hiding this comment.
Not ignoring marshalling errors anymore. (If any)
| } | ||
| if len(actual) == 0 || len(expected) == 0 { | ||
| t.Fatalf("%s json diff failed. Expected %d bytes in body, but got %d.", description, len(expected), len(actual)) | ||
| return fmt.Errorf("%s json diff failed. Expected %d bytes in body, but got %d.", description, len(expected), len(actual)) |
There was a problem hiding this comment.
Now that diffJson() returns an error, no need to differentiate this error asFatalf over Errorf. This particular JSON test won't continue to run because our test framework will simply go to the next JSON file in the directory.
| @@ -263,60 +282,54 @@ func diffBids(t *testing.T, description string, actual *adapters.TypedBid, expec | |||
| return | |||
| } | |||
|
|
|||
| diffStrings(t, fmt.Sprintf("%s.type", description), string(actual.BidType), string(expected.Type)) | |||
| diffOrtbBids(t, fmt.Sprintf("%s.bid", description), actual.Bid, expected.Bid) | |||
| assert.Equal(t, string(expected.Type), string(actual.BidType), fmt.Sprintf(`%s.type "%s" does not match expected "%s."`, description, string(actual.BidType), string(expected.Type))) | |||
There was a problem hiding this comment.
No need for diffStrings() anymore
| } | ||
|
|
||
| diffStrings(t, fmt.Sprintf("%s.uri", description), actual.Uri, expected.Uri) |
There was a problem hiding this comment.
No need for diffStrings() anymore
| for j := 0; j < len(actual); j++ { | ||
| if err = diffHttpRequests(fmt.Sprintf("%s: httpRequest[%d]", filename, i), actual[j], &(expected[i].Request)); err == nil { | ||
| break | ||
| } |
There was a problem hiding this comment.
Not sure about this approach. Please correct me if I'm wrong. For example it may not work correctly in case expected has 2 same exact requests (IDK if this is the case) and actual has also 2 requests, first one is correct and second one is empty(or different/incorrect). Bacause you start iteration over actual from the beginning - such case will pass.
I feel like you need to remove matched actual request from collection after it matched before iterating over it again.
I was not able to create a unit tests for this, becuse in most cases we only have 1 expected request. I can try to do this when 33across changes will be merged.
This test needs a test :)
There was a problem hiding this comment.
I think Veronika is correct in bringing up this example that wouldn't work. I'm also not sure if this behavior of 2 same requests is possible, but the solution to remove matched request from list makes sense to me!
Another idea could be to maybe convert expected and actual lists to be maps, which could make the comparison of these objects faster in general and solve the issue Veronika brought up.
There was a problem hiding this comment.
@AlexBVolcy Maps instead of a nested for loop was my first choice but I tried map[string]*RequestData but couldn't get the keys to match. Maybe I need to try again because I definitely like the hash table approach better.
There was a problem hiding this comment.
Not sure about this approach. Please correct me if I'm wrong. For example it may not work correctly in case
expectedhas 2 same exact requests (IDK if this is the case) and actual has also 2 requests, first one is correct and second one is empty(or different/incorrect).
You are right, this approach was implemented under the assumption that the expected entries at least differ in the root.imp[i].id fields but a corner case should be considered where this is not true. I'll give the remove matched actual request approach a try.
|
Added test looks good |
* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...
Current version of test framework function
assertMakeRequestsOutput(t *testing.T, filename string, actual []*adapters.RequestData, expected []httpCall)throws a false negative when the order of the entries in the parameter slicesactualandexpecteddiffer even when they come with matching elements. This pull request implements a correction that runs in O(n^2) time but, because tests are not run in production, we don't mind the extra processing time given that most tests come with slices of size 1 anyways.