Adds cookie attribute assertions (fixes #259) #261
Conversation
Adds the possibility to assert cookie attributes with the following
syntax:
```
expect(res).to.have.cookie('key', 'value', {
'AttrOne': 'value-one',
'AttrTwo': 'value-two',
// ...
});
```
When using .cookie(key, val, attributes), the assertion context will change from the original object to the cookie itself, in order to allow readable chained assertions about the cookie properties.
There was a problem hiding this comment.
I like the general progress here but I have some very general notes about the design:
-
I think adding
flag(cookieCtx, 'object', cookie)is a great idea, then we can simply use the power of chai's chainging to assert further on the object. -
I think the
.attribute()chain, along with therawCookieflag are not necessary..attribute()offers little over the already well established.propertyand I think it makes this feature more confusing to use. I think we should drop.attribute()and make sure the default.property()chain is ergonomic enough. -
getCookieAttributelooks fragile. I would much rather just seeflag(cookieCtx, 'object', rawCookieToObj(rawCookie))- so a fully parsed object comes back as the new assertion object, allowing forexpect(res).to.have.cookie('session').with.property('domain', '.abc.xyz'). -
You quite rightly point out the cookiejar lib sets "sensible defaults" which makes it difficult to assert on the exact cookie settings, but on L438 we fall back to using the cookiejar cookie to assert on. I think we should pick an opinion: either we're super strict and fail hard, or we're super leaniant and just use cookiejar all the time. I would err on the side of strictness, and simply fail an assertion that cannot manually parse a cookie, with an explainer as to why.
|
Hey @schrodervictor, hope you're having a good start to the holiday season. Have you had any time to look over Keith's notes? |
|
Hey @austince and @keithamus, sorry for the silence, but you know how the end of the year can be at work... |
|
No worries, I'm in that boat as well - good luck! |
|
Hey @keithamus and @austince, I want to get back to this topic and address your comments. So, I'll remove the newly introduced One thing we lose for sure is case insensitive comparison, but I don't see it as a crucial feature and it may be good to have more strict assertions. I don't share your opinion about About the defaults coming from cookiejar, well that's really annoying and not only that, there are only a limited number of cookie attributes it recognizes, the others are silently ignored. I would rather drop the support to assert properties of request cookies and focus only on response cookies. What do you think? One last, but important note: this whole idea introduces a small risk of breaking changes for some preexisting suites. Here is the situation: if a current test suite relies on the fact that the context doesn't change after invoking So maybe it's better to include this change in a major release. Another option would be to return the context to the request in case a |
Please refer to the README and comments in the pull request for greater details. Basically, this is adding the possibility to pass an object as the third argument to
.cookie(...)to assert about a certain subset of the cookie attributes, likePath,Domain,Max-Age,HttpOnly, and etc.It also adds a new assertion method chainable after a cookie assertion, to assert about a single attribute in a more "chaiish" readable style.
This pull request addresses issue #259.