Skip to content

test: add domain property tests for Cookie class - #1101

Open
Ayoub-Mabrouk wants to merge 1 commit into
expressjs:masterfrom
Ayoub-Mabrouk:test/add-domain-property-tests
Open

Ayoub-Mabrouk wants to merge 1 commit into
expressjs:masterfrom
Ayoub-Mabrouk:test/add-domain-property-tests

Conversation

@Ayoub-Mabrouk

Copy link
Copy Markdown
Contributor

Test setting domain to a value (e.g., 'example.com')
Test default value (should be undefined)

@yxshee yxshee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this branch: 149 passing and eslint clean. The two new tests also pass on master, which is expected for a coverage-only PR.

I tried a quick mutation to see what they protect. I deleted domain: this.domain from the Cookie#data getter in session/cookie.js, so Domain is never sent in Set-Cookie. With that change, the full suite including these two tests still passes (175 on current master plus this file). new Cookie({ domain: 'example.com' }).serialize('sid', 'x') then returns sid=x; Path=/; HttpOnly with no Domain. Both tests read cookie.domain, which the generic for (var key in options) copy in the constructor sets for any key, so they'd pass for a made-up option too.

One extra assertion would cover the part that can actually break, for example:

assert.strictEqual(cookie.data.domain, 'example.com')
assert.ok(/; Domain=example\.com/.test(cookie.serialize('sid', 'x')))

Also, this and #1100 insert at the same spot after the priority block, so whichever lands second will conflict in test/cookie.js. I checked by merging both onto master. The branch is also 15 commits behind master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants