.deep and .not are permanent?

[andyearnshaw]

I wrote the following test for a function that I expect to assign an object's properties to another object (in a similar vein to Object.assign):

expect(setPrivs(target, props)).not.to.equal(props).and.to.deep.equal(props);

This fails, even if I switch the order (deep before not). If I separate out the expressions, however, it passes:

expect(setPrivs(target, props)).not.to.equal(props);
expect(setPrivs(target, props)).to.deep.equal(props);

Looking at the documentation, I can understand why it fails. Both deep and not apply to any chains following and there appears to be no way to unset them that I can see. Looking at the natural language of my expression, it makes sense that the and chain should unset them.

I realise that this would be a breaking change, so perhaps an alternative would be to introduce but:

expect(setPrivs(target, props)).not.to.equal(props).but.to.deep.equal(props);

This behaviour would be very natural as "but" is "used to introduce a phrase or clause contrasting with what has already been mentioned."

Hopefully this makes sense and I haven't overlooked something.

[keithamus]

Hey @andyearnshaw thanks for the issue.

Indeed, at current the flags are passed along the chain - which is precisely why we don't have but. Introducing such a thing has been discussed before - I'm personally feel like it is a bit of a slippery slope, and possibly has too many edge cases to consider. But I'll leave this open for further discussion.

[andyearnshaw]

@keithamus do you have any links to previous discussions so I can get some context? I looked before I posted but found nothing.

[lucasfcosta]

Hello guys, thanks for your issue @andyearnshaw!
Actually, we do have .but. It was introduced as a no-op word due to this issue: #621. In that issue we would like to be able to write consistent phrases using .not only for the second assertion (which would be by in that case).

At that time I didn't think about assigning a behavior to it, but it does make sense to make it cancel any previous not statements.

Another possible solution for this is "consuming" the negate flag as soon as the first assertion after it has run, but that would be too confuse and would introduce a hidden and "magical" behavior IMO.

I can't (right away) think of any edge cases but (with the behavior described by @andyearnshaw) would introduce bugs or undesired behavior, because if we used .not right after it the negate flag would be turned on again.

I think it would be a great addition, what do you guys think?
However, if we all agree upon this feature I'd like to have more time to really investigate the implications of this change, since what I've written above is just what I was able to remember without looking at the whole code.

[keithamus]

I think the problem with something as innocuous as but: it dramatically changes the whole assertion, which could lead to lots of undesired behaviour. The current behaviour - while not perfect - is well defined enough that you can simply work around it.

Part of the problem is we hit a wall with the flexibility of the English language. but isn't used in such a strictly defined way - as would befit our current architecture. While one definition of but is to contrast the previous context (which fits the proposed definition here), another is to offer preposition. For example expect this object to have this property, but it not be a number is valid english, but could break code for us.

A more concrete edge case is already demonstrated in #621:

expect(fn).to.change(obj, 'key').but.not.by(2)

If but were to clear out all flags, it would clear all of the flags set by change, meaning by has no idea what to do. Here but is used prepositionally - to extend the assertion without contrasting it.

If we can work around issues like these, then I think we could look into this further, but my opinion right now is "this is not workable in our current setup" because of the above.

[andyearnshaw]

Thanks, guys. I wasn't aware but existed, but that use of it does indeed make sense.

If but were to clear out all flags, it would clear all of the flags set by change, meaning by has no idea what to do. Here but is used prepositionally - to extend the assertion without contrasting it.

There don't seem to be any sensible language chains (that I can see) where cancelling the change flags with but would make sense, so what if but just cleared the not and deep flags? An argument could be made for all and any too, though you could get something ambiguous-looking like the following:

expect(foo).not.to.contain.any.keys('a', 'b', 'c').but.keys('d', 'e', 'f');

It might make sense that explicitly specifying all or any in the second part should be a requirement (to prevent the obvious footgun).

[keithamus]

so what if but just cleared the not and deep flags

This again, to me, is a slippery slope. Having a flag cancel out some flags but not others seems like something that would be difficult to articulate properly in the docs, without an influx of issues coming about edge cases. If we are going to add such a feature, I think it has to be something that can be explained in one sentence, like "this flag clears out all previously set flags".

[andyearnshaw]

I can't say that I agree, I think it's easy to reason that but negates
not at the very least, yet makes no sense to negate things like have,
contains or change. I do understand your concern about edge cases, but
we appear to be hard-pressed to think of any and could they be worse than
the current, unchainable behaviour?

One possible safe alternative would be to introduce single flag-eliminating
chains, such as does:

expect(foo).to.not.equal(bar).but.does.deep.equal(bar);

shallow doesn't semantically undo deep, though. Perhaps exactly
could.

On Tue, 6 Sep 2016, 16:58 Keith Cirkel, notifications@github.com wrote:

so what if but just cleared the not and deep flags

This again, to me, is a slippery slope. Having a flag cancel out some
flags but not others seems like something that would be difficult to
articulate properly in the docs, without an influx of issues coming about
edge cases. If we are going to add such a feature, I think it has to be
something that can be explained in one sentence, like "this flag clears out
all previously set flags".

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#785 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvdEw9naiaDMUcoUVeNIj1UpZFalxPVks5qnY2pgaJpZM4J1k1I
.

[lucasfcosta]

Thanks for sharing your opinions, we're having a nice discussion here 😄

After trying to write some examples I ended up agreeing with @keithamus.
I still think that adding .but would not be a problem if it only canceled the .not flags the came before it (not anything beyond that), but I see no point in doing that, since when you are doing assertions you want to assert that something has a property X or does not have a property Y and these should be two different assertions, since one is the opposite of the other.

It makes no sense to say that you expect(something).to.not.be.equal(10).but.be.equal(20) because one expectation "cancels" the other.

Even when doing assertions like expect(anArray).to.not.include.members('memberOne').but.to.have.length(10) it doesn't look right. It would work, but the word but here doesn't necessary means contrast, since we are dealing with two different things. It also doesn't look as explicit as separating it in two different assertions.

I think that having but would encourage people to write assertions in the wrong way, because every assertion they make should be about one thing, obviously they can keep chaining assertions, but the chained assertions would refer the same subject, for example: expect(obj).to.have.property('name').that.is.a('string').

I think .but could be useful, but I'm just saying it isn't worth the risk of adding misleading or confusing behavior just to make it work for a small number of use cases (which don't seem exactly right for me).

[meeber]

In general, I strongly promote a one-assertion-per-block style of testing.

Conceptually, if I wanted to test if something was green and tasty, then my code would be structured as follows:

describe("something", () => {
  let something;

  before(() => {
    something = someFn();
  });

  it("is green", () => {
    expect(something).to.be.green;
  });

  it("is tasty", () => {
    expect(something).to.be.tasty;
  });
});

I prefer this style of testing over more concise alternatives that perform both assertions inside the same it block or even on the same line using language chains. The main reason I prefer the more verbose style is because if the first assertion fails, I usually still want to know if the second assertion passes or fails. A secondary reason for my preference is that the verbose style typically offers more descriptive output when running the test suite.

With all of that said, there are occasions when we need to perform two or more assertions in order to test a single concept. More importantly, there are occasions when there's no point in running the second assertion when the first assertion fails. @andyearnshaw's original example is such an occasion:

expect(setPrivs(target, props)).not.to.equal(props).and.to.deep.equal(props);

It's appropriate to include both of these assertions inside a single block. And it's more convenient to be able to do so by combining the assertions in a single line using language chains.

But is the convenience worth a breaking change to .but? My suspicion is no, as I believe the "not this, but instead this" construct to be rare. Does it have any uses beyond the specific "not equal, but instead deep equal" example? I don't like the keys example as there should be enough certainty in the test conditions to specify all of the keys that are expected instead of having to specify some keys that aren't expected and some keys that are expected.

I think I'd be more accepting of a non-breaking change, such as adding a new language chain like .instead that canceled the negate flag, but even then I'm not sure there's enough application for this to justify the change.

[lucasfcosta]

@meeber great explanation! I totally agree!
However, I'd just like to say that this would not be exactly a breaking change since we introduced .but after 3.5.0 and we didn't release anything since that.

Anyway, take @meeber's words as mine, I share his thougths 😄

[andyearnshaw]

I'll concede that but would only be useful to negate .not.equal when used with .deep.equal, as when deep isn't used you can put not in the final assertion's chain and just use and, at least with the current API surface. In it's current form, you can still use it as a natural language chain instead of and:

expect(5).to.be.above(0).but.not.above(10);

It would be nice if but would negate not, but I agree it's a minor use case and a minor annoyance.

In any case, thanks for the comments, I really enjoyed the discussion 😄.

[lucasfcosta]

Thank you too for bringing this proposal @andyearnshaw!
Even though we we didn't decide to add it to Chai it was good to hear other opinions and develop some thoughts on it.
If you still want to use .but with the behavior you just described, feel free to create a plugin and share it with the world, just in case anyone else wants that too hahaha 😄

For now I'll close this since we all agreed on this matter.