-
Notifications
You must be signed in to change notification settings - Fork 749
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
PBS Version In Response Header #2129
Conversation
endpoint(recorder, request, nil) | ||
assert.Equalf(t, test.expectedID, actualAmpObject.Request.ID, "Bid Request ID is incorrect: %s\n", test.description) | ||
} | ||
} | ||
|
||
func AmpObjectTestSetup(t *testing.T, inTagId string, inStoredRequest json.RawMessage, generateRequestID bool) (*analytics.AmpObject, httprouter.Handle) { | ||
func ampObjectTestSetup(t *testing.T, inTagId string, inStoredRequest json.RawMessage, generateRequestID bool) (*analytics.AmpObject, httprouter.Handle) { |
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.
There was no need to export this method.
t.Fatalf("Failed to fetch a valid request: %v", err) | ||
} | ||
reqBody := string(getRequestPayload(t, reqData)) | ||
reqBody := readVideoTestFile(t, "sample-requests/video/video_valid_sample.json") |
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.
Added a test loading helper method like the auction and amp endpoint have.
sb.WriteString(name) | ||
sb.WriteString("/") | ||
sb.WriteString(version) | ||
} |
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.
Copied/pasted code from another file. No change to BuildXPrebidHeaderForRequest or writeNameVersionRecord.
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.
This looks good to me! I checked code coverage against the new additions you made and that looks good too. I'm going to wait until Gus reviews to see if I may have missed something.
description string | ||
requestURLArguments string | ||
expectedStatus int | ||
expectedHeaders func(http.Header) |
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.
For my own understanding, what is happening here with func(http.Header)
? I haven't seen this syntax with func(something)
and see it in a couple of places here in this PR.
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.
Good question. This is a function pointer. It's written in the same format as a normal Go function header, but I'm omitting the variable name since it doesn't matter in this context. In the test cases, I'm defining anonymous functions which need a variable name for use in the body.
This is so that I can use the .Set()
method for adding headers to the expectedHeaders
map. Since the underlying data structure is a map, I could get away with a map literal - but I prefer to use the exported methods and interact with the http.Header
data structure in the exact same way the real code does by making calls to the .Set()
meoth.
I could also use a map and loop through with making calls to .Set()
, but I thought this was easier and more direct. It's an implementation choice and I wouldn't comment on anyone using an alternate approach. It's just that this is what looks good / makes sense to me.
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.
Looks pretty good. I'm ready to approve after merge conflicts get resolved.
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.
LGTM
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.
LGTM
Implements #2010