Conversation
All cookie keys were being lower-cased before comparison, but actually case should be preserved for the first key (the cookie-name). See also: [RFC 6265](https://datatracker.ietf.org/doc/html/rfc6265)
A1exKH
left a comment
A1exKH
left a comment
There was a problem hiding this comment.
Overall idea of this test looks good, to finalize this code review we need some additional information from author of PR.
test/cookies.js
Outdated
| .expect('Content-Length', '15') | ||
| .expect(200) | ||
| // assert 'Alpha' cookie is set with domain, path, and httpOnly options | ||
| .expect(cookies.set({ name: 'Alpha', options: ['domain', 'path', 'httponly'] })) |
There was a problem hiding this comment.
@dmurvihill could you share more details about this code, please?
As I can see cookies.set and cookies is not a standard global object in Supertest.
In standard Supertest, you cannot pass an object like { name: 'Alpha' } directly into .expect(). Instead, you have to parse the Set-Cookie header string.
Here you can find some details:
#665
There was a problem hiding this comment.
cookies was added in supertest 7.2 (#855). See cookies.set.
test/cookies.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('Cookie name case is respected', function () { |
There was a problem hiding this comment.
What do you think about this version of code to make it clearer:
describe('Cookie Case Sensitivity', function () {
it('should respect capital letters in cookie names', async function () {
There was a problem hiding this comment.
Renamed to respects case of cookie name and moved up with the rest of the tests for .set.
A1exKH
left a comment
A1exKH
left a comment
There was a problem hiding this comment.
LGTM.
@dmurvihill thanks for information and updates!
3 participants
Includes @caarmen's test from #880 as well as a fix for the underlying problem.
Checklist