镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

test: add sameSite property tests for Cookie class - #1100

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

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

Conversation

@Ayoub-Mabrouk

Copy link
Copy Markdown
Contributor

Test setting sameSite to 'strict'
Test setting sameSite to 'lax'
Test setting sameSite to 'none'
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: 151 passing and eslint clean. The four new tests also pass on master, as expected for tests-only.

Like the domain tests in #1101, these read cookie.sameSite, which the constructor's generic for (var key in options) copy sets for any key. I deleted sameSite: this.sameSite from the Cookie#data getter (so SameSite is never serialized) and ran this file: all 22 tests in test/cookie.js still pass. On current master the regression is still caught, but only by 11 sameSite: 'auto' tests in test/session.js.

If the goal is to pin down the Cookie class itself, asserting the serialized value would make these tests catch that, e.g. new Cookie({ sameSite: 'lax' }).serialize('sid', 'x') contains ; SameSite=Lax. The boolean forms are also documented and currently untested at this level: true gives SameSite=Strict and false omits the attribute (both checked on master).

This and #1101 add their blocks at the same place in test/cookie.js, so the second one merged will need a rebase. Merging both onto master gives a conflict.

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