chai-http
chai-http copied to clipboard
Adds cookie attribute assertions (fixes #259)
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.
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... 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.
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 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.