Flawed tests that pass instead of alerting user of an issue

[meeber]

We have a problem in Chai. The problem is that it's too easy to write tests that appear valid but are actually broken in such a way that they pass no matter what. Strict TDD/BDD can help mitigate this problem, but it's not enough, especially when reviewing PRs from developers of varying skill levels.

Before discussing solutions, it's important to understand the different ways this problem can manifest. Here are three:

describe("flawed tests that always pass instead of alerting user of an issue", () => {

  // Problem A
  it("passes no matter what because of a typo", () => {
    expect(x).to.be.ture;
  });

  // Problem B
  it("passes no matter what because no assertion is actually made", () => {
    expect(x).to.equal;
  });

  // Problem C
  it("passes no matter what because of mishandled asychronicity", () => {
    setTimeout(() => expect(x).to.equal(y), 1000);
  });

});

Commonly proposed solutions include:

1. Replace all property assertions with method assertions (see: dirty-chai).
2. Use ES6 Proxy to detect invalid assertions (see: Throw when non-existent property is read #721).
3. Add a mechanism to verify that the expected number of assertions were run in each test (see: chai-checkmark).

None of these solutions are mutually exclusive. Here are some pros and cons of each approach:

Replace all property assertions with method assertions

Pros:

• Allows linting tools to be used to automatically identify Problems A and B.
• Decreases chances of Problems A and B from occurring in the first place since developers get used to ending every assertion with a method.
• Makes it easier for reviewers to visually spot Problems A and B for the same reason.
• Easy for Chai to implement.
• Easy for developers to use.
• Works in any version of JavaScript.

Cons:

• Not just a breaking change, but a devastating one, invalidating every single test that uses property assertions. Worse yet, it breaks them silently, causing them to pass no matter what due to the same issue as Problem B. On the plus side, developers who read the patch notes will know to re-enable the linting rule that flags property assertions, but anyone who doesn't is screwed without them even knowing it.
• Breaks any plugin using property assertions.
• Doesn't address Problem C.
• Doesn't actually fix Problems A and B; merely decreases odds of them happening as well as allows the use of a linting tool to detect them.
• Method assertions aren't as pretty as property assertions.

Use ES6 Proxy to detect invalid assertions

Pros:

• Automatically fixes Problem A.
• Easy for Chai to implement.
• Easy for developers to use.
• Causes few-if-any breaking changes.

Cons:

• Doesn't address Problems B or C.
• Only works in recent versions of JavaScript; older versions automatically fall back to current Chai functionality.
• Could have performance impact in extreme cases? (unknown)

Add a mechanism to verify that the expected number of assertions were run in each test

Pros:

• Fixes Problems A, B, and C if consumer uses it correctly for each test.
• Causes few-if-any breaking changes.
• Works in any version of JavaScript.

Cons:

• Difficult for Chai to implement because of chaining multiple assertions together, the ability for assertion chains to be branched into separate chains, the tendency for assertions to actually call multiple assertions underneath the hood, and the lack of tight integration between test runner and assertion library.
• Difficult for developers to use because of the above reasons and because they potentially have to add extra code to every test.

Note: A variation of this approach is to merely verify that at least one assertion was run in each test, instead of a specific number, which would be potentially easier for Chai to implement and developers to use, but would mean that it no longer completely fixes any of the problems, only decreases the likelihood of them occurring.

[meeber]

If this project was new, I'd feel like replacing all property assertions with method assertions would be the obvious correct choice. But given the maturity of Chai, I'm worried about how extreme of a breaking change it is. I'm currently undecided. If we do go with this approach, I'd say that it should not only be released in a new major version, but that it should be the only change introduced in the entire major version, both to make it easier for consumers to pick which version they want to use based on this one specific change, as well as to make it easier for consumers to update their code without having to worry about more than one change at a time.

Although they're not mutually exclusive, I'm only in favor of using ES6 Proxy to detect invalid assertions if we don't replace all property assertions with method assertions. It feels pretty cool and didn't end up being nearly as complex as I feared, but I'm a bit uncomfortable with having the code path be different based on the JavaScript version. Still, I think Problem A is a big enough problem that if we don't fix it by replacing all property assertions with method assertions, then we should fix it with ES6 proxy. I don't think Problem B is nearly as prevalent, so I'm not particularly concerned that this approach doesn't address it.

As for adding a mechanism to verify that the expected number of assertions were run in each test... I think this has a lot of potential as a cool feature, but will be difficult to do correctly as explained in my first post. I'm in favor of eventually working something like this into Chai core regardless of what we do with property assertions and ES6 Proxy. After all, it addresses one of the problems that neither of the other two approaches does.

In conclusion, I'm in favor of either replacing all property assertions with method assertions, or using ES6 Proxy to detect invalid assertions, but not both. Not sure which I prefer yet. Regardless, I'm also in favor of eventually adding a mechanism to verify that the expected number of assertions were run in each test, but I consider this a longer term addition.

Edit: One of the biggest advantages of replacing all property assertions with method assertions is that it allows consumers to use a linting tool to catch invalid assertions. But it's possible for a linting tool to provide that service anyway, without replacing property assertions. A custom ESLint plugin for Chai would likely be the best solution. Looks like there's already one in the works: eslint-plugin-chai-expect.

[keithamus]

I think I'd like to get rid of property assertions - they have many different types of bugs, from odd stack traces, to confusing developers (both plugin developers and users writing tests). Also in the roadmap issue I briefly mentioned some of the impact these property assertions have:

Two of the biggest causes of fragmentation of chai users is due to low browser support, and the property assertions - which spawns off codebases like the following:

• hapijs/code
• rowanmanning/proclaim
• moll/js-must
• wrumsby/ristretto

I think we can mitigate the pain of removing them by spreading the changes over several major versions, a loose plan would be:

• Reach the point where interfaces are fully pluggable, and can be their own modules (probably needs Refactor plugin declarations #585)
• Create interfaces for should/expect which do not have property assertions
• Add the method only interfaces in chai core, under a namespace, e.g. import expect from "expect/methods", at the same time have import expect from "expect/old" which loads the old one. import expect from "expect" is an alias for "expect/old"
• Eventually import expect from "expect" aliases "expect/methods" but we still keep "expect/old" - users who want to keep the property assertions can just change 1 LOC (the import statement) and not have to change any of their test code
• Maybe, one day, when we're a few major version ahead, and confident most people are using "expect/method", we drop the "expect/old" interface altogether.

[meeber]

I'm on board for removing property assertions, and following a gradual approach in doing so, but in that case I think we should consider implementing the proxy stuff in the short term (even though that contradicts what I said earlier about not doing both). I think it'll be a decent chunk of time until #585 is in place, whereas I consider Problem A in my original post fairly urgent.

[keithamus]

Ultimately I think all 3 solutions could be used. We've definitely had requests for counting assertions before (not necessarily because of property assertions though). But yes I agree, proxies will solve problems the quickest with (hopefully) the least impact.

[peoro]

+1

I was about to open the same issue right now.
Sometimes I mistype exist as exits or exists without ever realizing, and too often I need to open the Chai documentation just to check whether NaN is a valid property, whether it's called nan or NaN or similar stuff.

All the proposed solutions would work great for me.
I'd have a further idea about the first solution: maybe the property assertions could return a noop function, rather than the object they're currently returning? This way former code can keep using them as property assertions, while new code could call them as functions to be sure the properties weren't misspelled. Not sure whether this would break the API though.

[keithamus]

I'd have a further idea about the first solution: maybe the property assertions could return a noop function, rather than the object they're currently returning? This way former code can keep using them as property assertions, while new code could call them as functions to be sure the properties weren't misspelled. Not sure whether this would break the API though.

We did this before, around Chai 1.10.0. It didn't end well, because it causes all kinds of issues with chaining of assertions. You can read the full story here: #371 (comment). Here's the release notes for Chai 2.0 which removed this: https://github.com/chaijs/chai/releases/tag/2.0.0

[andyearnshaw]

We did this before, around Chai 1.10.0. It didn't end well, because it causes all kinds of issues with chaining of assertions.

What kinds of issues? If, instead of a noop, the assertion simply returns itself when called, wouldn't that cover those kinds of issues? If a proxy were used to check if an assertion is callable in the {{apply()}} trap (returning the assertion itself if not callable), perhaps it would be possible to "fix" existing plugins while at the same time retaining compatibility.

Property assertions frustrated me to the point of enforcing stricter linting rules for work projects, as several tests were passing when they should have failed due to typos or incorrect assumptions about the assertions available.

[meeber]

@andyearnshaw The good news is, starting with Chai 4.x.x, an error will be thrown when an invalid property assertion is used (due to type or incorrect assumption). See: #721

The best place to read about the issues with the previous attempt at allowing both property and method assertions is: #302, particularly #302 (comment)

I think one could make an argument that it's worth supporting both property and method assertions if the only thing that breaks is assertions such as expect([1,2,3]).exist.length(3) where
the .length or .arguments assertion immediately follows the chainable no-op, not allowing anything in between to return an assertion object instead of the no-op function. This may be more palatable than getting rid of property assertions altogether in favor of method assertions.

I'm not sure the idea you've proposed with proxies is viable. My understanding is that the Apply trap will only work if the thing being proxied is a function. In other words, it can't be used as a mechanism to detect if something that's being called is actually callable. Please correct me though if I'm misunderstanding your idea (or if I have incorrect knowledge of the proxy Apply trap).

[andyearnshaw]

@meeber: ah yes, it looks like you're correct. I recall an early implementation in Chrome allowed this, but now that I've checked the spec I can see that it's wrong.

Glad to see the throw for invalid property assertions coming in 4.x.x, that might change my mind about them!

[tristanisme]

Here's a trap I just fell in. Sorry if you're already aware of it.

myBool.should.equal.true;

This looks correct and makes intuitive sense, at least to me as a newcomer, but doesn't test anything.

[lucasfcosta]

Hi @tristanisme, thanks for your report!
The equal assertion is only added as a method, the correct way of writing it should be myBool.should.equal(true) or myBool.should.be.true.
In your case I suppose your assertion is returning undefined, because you are trying to access the property true of the equal method the Assertion object has.

However, your syntax consideration seems common and natural enough for me to think it should be considered correct. IMO this behavior should be added to Chai's core. Perhaps we could make equal have a different behavior by defining its getter function in order to enable our users to do this kind of assertion, but this feels a little weird to me, because the same word would have two different effects and this could end up confusing other users.

If we really wanted to do this I think it would only be a matter of using addChainableMethod .

I'd like to hear @keithamus and @meeber opinions on this one, what do you guys think?
Also, if you think this is going to be a long discussion we can have a separate issue to talk about it.

[keithamus]

Proxies should solve this, come 4.0 - statements like that will throw an error.

As for supporting those, IMO I'd rather steer away from property assertions than into them.

[vieiralucas]

@keithamus I just tested here using the 4.x.x branch and I don't think that Proxies would solve this case.
Even if he types myBool.should.equal.tue; (missing the r).
The Invalid Chai property error will not be throwed because the equal property is not proxified. I did not digged into the code but I believe that only properties are being proxified.

What I did to test this was:
Checkout to the 4.x.x branch
Add the example to a random test
run npm test
And the tests passed

Please correct me if I'm wrong or if my method for testing this is wrong.

[keithamus]

@vieiralucas we're in a bit of a mixed state right now. I have a vague feeling that Proxies might be in the master branch, while the 4.x.x branch contains other things. @meeber will probably have a better idea on this though.

[meeber]

@lucasfcosta and @vieiralucas are correct that proxies won't catch this particular problem. Since the equal assertion is a plain method instead of a chainable method, .equal.true and .equal.ture are just accessing undefined properties on a regular function, so they silently do nothing.

[meeber]

It's also worth mentioning that something like expect(x).to.equal.true is a more severe example of Problem B from my original post, because unlike expect(x).to.equal, it looks like a valid assertion. This makes solving Problem B more urgent, nearly as urgent as solving Problem A with proxies was.

[keithamus]

We can use proxies to wrap functions so we can mirror the same behaviour that we have with objects now, on the functions.

[meeber]

@keithamus Great idea. In fact, I wonder if an attempt to access any property on a non-chainable method assertion should throw an error, even if the property exists. For example expect(x).to.equal.length could throw an error even though the function has a built-in .length property.

[lucasfcosta]

Great idea by @keithamus, +1 for that!
Regarding @meeber's last example, I'm not sure it would be a good idea, since it would be kind of conflicting with the language's standard features. Even though we won't see many people doing that I think that your example would still be a valid construction and by doing it I (as an user) would expect to see the value of the length property of the equal function. However, I don't have a very strong opinion about it, let me know if you disagree or if you have any other ideas you'd like to share. 😄

[meeber]

The release of Chai v4 with proxy protection drastically mitigated this problem. Gonna close.