Skip to content
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

Adds cookie attribute assertions (fixes #259) #261

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

schrodervictor
Copy link

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, like Path, 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.

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.
Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

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 the rawCookie flag are not necessary. .attribute() offers little over the already well established .property and 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.

  • getCookieAttribute looks fragile. I would much rather just see flag(cookieCtx, 'object', rawCookieToObj(rawCookie)) - so a fully parsed object comes back as the new assertion object, allowing for expect(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.

@austince
Copy link
Contributor

Hey @schrodervictor, hope you're having a good start to the holiday season. Have you had any time to look over Keith's notes?

@schrodervictor
Copy link
Author

Hey @austince and @keithamus, sorry for the silence, but you know how the end of the year can be at work...
I haven't had time yet to adjust the pull request and reply to the comments, but I'll do it. I hope I can do it at some point before the year ends.

@austince
Copy link
Contributor

austince commented Dec 13, 2019

No worries, I'm in that boat as well - good luck!

@schrodervictor
Copy link
Author

Hey @keithamus and @austince, I want to get back to this topic and address your comments. So, I'll remove the newly introduced attributes(...) method from the chain and see how it goes with testing with the default property(...).

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 getCookieAttribute being fragile as it seems pretty solid to me, but the other approach (having the fully parsed object) also works, so I'll follow your suggestion for this one as well, reducing the code complexity.

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 .to.have.cookie(...) to perform more assertions about the request, it will break after this pull request gets merged. Example:

expect(res).to.have.cookie('foo').and.to.have.cookie('bar');

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 .cookie(...), .header(...) or any other request/response specific assertion is attempted other than property. This is probably doable to avoid the breaking change and have a smoother release, but to be honest I'm not sure about the added complexity and what could still possibly still break after this. I would like to have your opinion before going down this hole.

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.

3 participants