Add support to better cookie assertions

[schrodervictor]

Hello!

We are using this library quite extensively, but we have the need to assert aspects about the cookies being set by the server, like Domain, Max-Age and Path, but I could see in the source that these are not currently supported.

I gladly offer myself to implement this feature and open the respective pull request, as I would need to fork and implement it anyways, but I'm opening this issue to ask if the contribution would be accepted and also to discuss the preferred interface for this kind of assertion.

My first idea would be to implement it as expect(req).to.have.cookie(key, value, info);, keeping both value and info as optional arguments to not break backwards compatibility. The argument info would be an object with the additional information to assert about the cookie. In my current example, it would be something like this:

expect(req).to.have.cookie('group-foo', 'a', {
  'Path': '/',
  'Max-Age': 3439574,
  'Domain': '.caseable.com',
  'Secure': true,
});

Sounds good?

[keithamus]

This looks like a great idea to me. @austince WDYT?

@schrodervictor it'd be good to have info only check the keys present in the arg, e.g. if I omit the Path key it can still assert on Max-Age, Domain and Secure. Make sense?

[austince]

I think this would be great too! I'm just also wondering about the info object - are those exact matches? I feel like people might want to use other types of assertions on those properties.

I'm not sure if this is possible, but would it be better to just improve access to cookies?

const fooGroupCookieInfo = expect(req).to.have.cookie('group-foo', 'a')
expect(fooGroupCookieInfo).to.have.property('Path', '/')

[schrodervictor]

@schrodervictor it'd be good to have info only check the keys present in the arg, e.g. if I omit the Path key it can still assert on Max-Age, Domain and Secure. Make sense?

@keithamus, that's exactly what I thought. If the assertion isn't mentioning an certain cookie attribute, we should consider it not relevant for the test case presented.

I think this would be great too! I'm just also wondering about the info object - are those exact matches? I feel like people might want to use other types of assertions on those properties.

@austince, I first thought them as exact matches, but doing regexp would be straight forward. Some attributes, like HttpOnly and Secure, have no value. They can only be present or not, so I thought to make them booleans (using true to assert they are present). Expires and Max-Age would probably be better checked as a value within a range, as exact matches may be very complicated most of the times.

I'm not sure if this is possible, but would it be better to just improve access to cookies?

const fooGroupCookieInfo = expect(req).to.have.cookie('group-foo', 'a')
expect(fooGroupCookieInfo).to.have.property('Path', '/')

You are probably right, as it would be more flexible, but I'm not sure how to do it. Do you have experience implementing this kind of enhancement? If yes, could you point me some directions?

The only detail I disagree is that I would definitely not capture the return value of a previous assertion and pass it through another expect as suggested in the example. It would be hard to use. From the API point of view, I would go more with the chaining offered by Chai.js, something like this:

expect(req).to.have.cookie('group-foo', 'a').that.has.property('Path', '/');
expect(req).to.have.cookie('group-foo', 'a').that.has.property('HttpOnly');

Anyways, I would say that the original proposal is fully compatible with this one and we could maybe have both. As for the first one, I would basically already know how to do it. This later one has way nicer interface and flexibility, but would require more research. We could do it in two steps. What do you think?

More inputs? Can I safely start?

[keithamus]

Please go ahead! I look forward to seeing the PR!

[austince]

I think the chaining option makes sense and will be easier to use as well! You may want to do exact matches for now and leave the regex/ ranges for a later extension, but totally up to you and thanks for the thorough plan!

[schrodervictor]

Thanks from pinging here from #255, @austince! No problem on my side with those changes, as I haven't started to code this feature yet. It will be nice to have CI in place for the upcoming pull request, :+1:

[schrodervictor]

@austince and @keithamus, the pull request is out, but it seems to have a problem with Github CI. Judging by the error message, it doesn't look like anything related to code itself, but more like configuration error or a third party thing (server error, 500...). Not sure. Could you please take a look? If any action is needed from my side, just let me know.