-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[tests] Properly test for multiple headers with same name in probes #9538
Conversation
Updates the `responseHeaders` probe checks to properly test for multiple headers with the same name. Previously the probes were using `headers.get()` which concats multiple headers into a single string, which results in the check not really checking if there are in fact multiple headers with the same name. Using `headers.raw()` allows us to properly test for that. A couple of Python tests that were already checking for multiple `set-cookie` headers needed to be updated to match the full value, since the check now properly validates the full string match of each header (before it was basically just doing a `string.includes()` check). This is a precursor for #9533.
const headers = Array.from(resp.headers.entries()) | ||
.map(([k, v]) => ` ${k}=${v}`) | ||
.join('\n'); | ||
const actualArr = rawHeaders[header.toLowerCase()]; |
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.
Are raw headers always lowercase keys?
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.
They are not.
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.
So why is the header.toLowerCase()
there? Are we assuming the server will always lowercase the key before sending the response?
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.
I guess so. Some existing tests were using lowercase.
Updates the
responseHeaders
probe checks to properly test for multiple headers with the same name.Previously the probes were using
headers.get()
which concats multiple headers into a single string, which results in the test not really checking if there are in fact multiple headers with the same name. Usingheaders.raw()
allows us to properly test for that.A couple of Python tests that were already checking for multiple
set-cookie
headers needed to be updated to match the full value, since the check now properly validates the full string match of each header (before it was basically just doing astring.includes()
check).This is a precursor for #9533.