Suggestion: detect expect() without matcher in core

[elliot-nelson]

Summary

If you've used jshint / eslint / tslint, you're probably encountered some variety of a rule that helps you detect this chestnut:

expect(x > 3);     // missing .toBe(true)!

// (ofc should use toBeGreaterThan, but at least toBe would assert something)

If you're not using a linter, though, to someone unfamiliar with javascript testing frameworks this looks totally valid and might sit in a large project for months/years without being detected. Probably until the code breaks and the test doesn't and someone investigates.

Possible solution

Add a simple boolean to Expectation that declares whether a matcher has been attached. Whenever a matcher has been called, set this flag to true (can add to Expectation's comparison wrappers). Then all Runnables (Spec/Suite) can be extended to add the result of every expectation factory call to a local list of expectations, and the QueueRunner can ask every runnable after it is finished "do you have any expectations with no matchers?", adding a new error to the spec/suite as appropriate.

Context

On larger teams, if you don't have some very strong linting enabled, you can fall into this trap a lot. I think maybe, it makes sense for Jasmine itself to consider an expectation with no matcher an error, since it is essentially a broken test.

The issue arises a lot when mixing different environments, for example, I've seen this before in jasmine + React tests:

const pane = mount(<MyPane />);
expect(pane.matchesElement(
    // 20 lines of JSX here
));   // Oops!
// "pane.matchesElement" kind of looks like a matcher, but this test doesn't assert anything

[slackersoft]

Given the existing tooling in the various linters to detect this, I'm not sure Jasmine wants to do similar work. This would require a pretty hefty re-working to how expectations are managed within Jasmine, since currently the first time a Spec knows that it has any calls to expect is when they report their result to it. (This isn't completely true, currently a Spec just delegates back to Env and there isn't really a reason currently to round-trip through the runnable when the expectationFactory could be called directly from within Env.expect.)

We might be open to something like this in the future, but right now the effort seems to outweigh the benefits.

[elliot-nelson]

Respect that! I'll make a final pitch for a much simplified version:

Every time you expect(), ask "lastExpect" if it had a matcher called on it. If it didn't, toss up a spec failure. Keep lastExpect around. This isn't 100% foolproof but the code for it could live pretty much just in the expectation factory created by Env.

(If that's still too much churn, I'm fine with closing this one for now... can always revisit if there's ever a big refactor in the future.)

[slackersoft]

Expectations just aren't really tracked in any real way until they report a result currently, so this would probably still be a decent sized undertaking. Also of note, keeping track of the lastExpect still has an issue if that is the last expect() in a given spec, which seems like the most likely place to forget your matcher call.

[elliot-nelson]

I have a reference implementation of this in a branch; I'll sit on it for now, but may put up a PR later this year just as a "how about this?" 😉.