Consider removing require-expect from the recommended ruleset

[jaswilli]

Hi,

I'd like to propose that require-expect be removed from the recommended set of rules. I think it's odd that we'd actually recommend against using assert.expect in tests as using it consistently makes test suites more robust and forbidding its use seems like a case of removing a set of guardrails because, "trust me, I know what I'm doing," which is sort of antithetical to the spirit of a linting tool.

I think #222 (comment) was perhaps a mischaracterization of assert-expect--I don't believe it's no longer needed nor redundant.

My actual preference would be to revert back to recommending except-simple, however, I realize that's unlikely to happen at this point 😄

Originally posted by @jaswilli in #381 (comment)

[bmish]

Out of curiosity, what kind of use cases do you see assert.expect(n) being valuable for? And are those use cases common?

Teams and codebases I have worked in moved to remove most usages of assert.expect(n) when we switched to native async tests/assertions, which eliminated much of the risk of a testing not hitting certain assertions.

And for me personally, I've always found assert.expect(n) to be highly burdensome in terms of having to manually keep the assertion count up-to-date. It can be painful to update or refactor tests when assert.expect(n) is present, which essentially adds an additional step / overhead of fixing the count to each test change. Rather, I typically strive to make tests easy to update/refactor/cleanup as behavior or needs change, and tests littered with assert.expect(n) slows that process down.

So I think that covers the reasoning behind the change of the require-expect rule default to never-except-zero in that:

1. assert.expect(n) is less necessary than before.
2. Its unnecessary usage can be burdensome.
3. In (rare?) cases where it's still necessary, or when the user prefers to always use assert.expect(n), the rule can always be customized or disabled.

What do you think?

CC: @Krinkle @sukima @jacobq @raycohen @mongoose700

[jaswilli]

Out of curiosity, what kind of use cases do you see assert.expect(n) being valuable for? And are those use cases common?

The two biggest use cases in my current set of projects are:

• Asserting something inside a request mocking handler, e.g., Mirage, where the test defines a request handler and wants to assert that it's been called and has the correct state.
• Asserting inside a mocked event/action handler. Obviously there are ways around this, like using sinon, but not everyone can or wants to use a third-party test helper library.

2. Its unnecessary usage can be burdensome.

This is absolutely just personal preference but I actually find the "burden" to be beneficial in that I think it discourages casual remove of test assertions. Maybe it's silly but I've always found the extra step of needing to go and decrement the expected assertion count makes me stop and consider whether I've just made the test worse.

3. In (rare?) cases where it's still necessary, or when the user prefers to always use assert.expect(n), the rule can always be customized or disabled

I agree that ultimately each project can configure this rule as they please, which is why I'm not really upset about the change in v8. It just seems like recommending that assert.expect not be used is perhaps a step too far and maybe it should just be removed from the recommended ruleset? That way we're not making a value judgement on assert.expect at all?

[platinumazure]

That way we're not making a value judgement on assert.expect at all?

I strongly prefer avoiding a value judgment on assert.expect if its best practices are not as clearly established as we thought. At the moment, removing the rule from the recommended configuration feels like the best option. But I am open to further discussion.

As noted, users are welcome to override individual rule settings in their project configs. So no one should be completely blocked by this. But I acknowledge it will be more convenient to remove the rule from the recommended configuration unless a clear consensus emerges here.

[amk221]

I also like requiring the use of assert.expect, since junior developers may not know to reach for assert.step and often end up writing tests that pass by accident.

[LucasHillDex]

I think an option for the rule that could be useful is similar to "except-simple" where assert.expect is required in tests with asserts in blocks etc, but all other assert.expects are errors. I think it makes sense as a default as well, because there are edge cases where you cannot write your test with async/await and this rule has been a good reminder in the past that you're introducing risk by not having assert.expect or assert.async.

[Krinkle]

I also like requiring the use of assert.expect, since junior developers may not know to reach for assert.step and often end up writing tests that pass by accident.

I agree. Howewer, I think this is what code review is for. (Or, potentially, a more advanced lint rule that can recommend use of assert.step when appropiate.)

The practice I generally follow is that test assertions only belong directly in the test function, not nested inside callbacks or otherwise produced in ways that are not directly under the control of the test function. This practice imho results in tests that are most stable, most valuable, and most strict.

Enforcing a rule that makes a badly written test slightly less bad, whilst making all other tests more difficult and cumbersome to develop and maintain, is imho not a good rule, and doesn't result in the junior engineer learning about why these tests are fragile. If we can develop a rule that more generally discourages placing assertions (apart from recording steps, which indeed does happen through the assert object today, unless you use your own array with deepEqual at the end), that might be worth persuing, but otherwise I think I lean towards not recommending this rule by default.

More generally, I think due to how widespread this practice is, I think rules like require-expect have a tendency to make QUnit unattractive to new engineers. From their perspective, I imagine, it simply appears that QUnit "needs" it, whilst others don't, thus making QUnit appear as if it is less reliable. This isn't true of course, but I can see how it might appear that way if the path of least resistence produces a lint configuration that tells them to place these numbers there.

I think an option for the rule that could be useful is similar to "except-simple" where assert.expect is required in tests with asserts in blocks etc, but all other assert.expects are errors. I think it makes sense as a default as well, because there are edge cases where you cannot write your test with async/await and this rule has been a good reminder in the past that you're introducing risk by not having assert.expect or assert.async.

Yeah, I think a lint rule to detect complex cases would be great. I would prefer such rule to recommend adoption of assert.step() as opposed to assert.expect() but either way is better than nothing. If there is enough interest in recommending the more loose assert.expect() approach, I'd be fine with that being an option as well.

Perhaps something like qunit/no-indirect-assertions: off | no | require-expect | require-step

[HeroicEric]

I don't think we should require using assert.expect() but we also should not explicitly disallow it. I agree with what @jaswilli said. There are cases where using assert.expect(n) is useful like assertions inside event handlers.

I know QUnit is not only used by Ember developers but this technique is even used in official Ember guides for testing components.

Is there a simple way to write a similar test without requiring another tool like Sinon?

[Krinkle]

I've raised the topic at https://discuss.emberjs.com/t/the-value-and-benefit-of-assert-expect/20145.

[esbanarango]

This is absolutely just personal preference but I actually find the "burden" to be beneficial in that I think it discourages casual remove of test assertions. Maybe it's silly but I've always found the extra step of needing to go and decrement the expected assertion count makes me stop and consider whether I've just made the test worse.

Can't agree more with this. I'm also a huge fan of the use of assert.expect(n). I find it easier to "read" when I see how many assertions a given test has. I treat it mostly as documentation, but it will also prevent deleting assertions by mistake when doing refactors.

[platinumazure]

Friendly reminder that this discussion isn't about removing the rule entirely-- just about removing the rule from the recommended configuration. Anyone who wants to keep using the rule can specify it explicitly in their ESLint configuration, in the rules section.

This discussion is about whether including the rule in recommended is causing too much inconvenience for users who have to explicitly disable the rule if they don't want it or change the rule because the default we chose was not sensible for most users.

The fact that people are arguing about whether assert.expect should be used, forbidden, or tolerated in this conversation tells me that we do not have a configuration that we can recommend to most users. So I lean even more strongly towards removing the rule from the recommended config.

The intention of the recommended config is "Don't know what rules to start with? Here are some clear best practices that pretty much everyone agrees are good to use in most cases". This clearly does not apply to our rule for assert.expect as we do not have that consensus. But again, we aren't getting rid of the rule completely-- we're just making it opt-in for users of the recommended config.

[Krinkle]

[…] this technique is even used in official Ember guides for testing components.

After discussion, this example has been updated in ember-learn/guides-source#1940.

Maybe it's silly but I've always found the extra step of needing to go and decrement the expected assertion count makes me stop and consider whether I've just made the test worse.

@jaswilli I don't think that's silly! This kind of "tactical frictional" is built-in to assert.step() as well.

I believe the steps approach has the added benefit of making the "decrement" more understandable in code review. E.g. no need to reason through the count, but you'd very clearly remove a specific string from the step list. With the steps approach we also limit this friction to dealing with these kind of "steps" e.g. from callbacks and other indirect execution. Regular top-level assertions in the test function (possibly with async-await) do not get this friction applied to them. In any case, projects are of course free to enable require-exports or even QUnit.config.requireExpects if you so choose.

Is there a simple way to write a similar test without requiring another tool like Sinon?

@HeroicEric I believe so, yes, with assert.step().

The Step API is significantly more robust and strict than assert.expect(), and also more strict than typical Sinon use. It is of course possible with more code to configure Sinon nearly-equally strict, but this would require complexity and verbosity in your test code, for a relatively small benefit. I imagine it would quickly feel as an outweighed cost to catch a regression that might never come, e.g. checking exact call count, and call arguments, and the order of calls, and then somehow verify that order across multiple different callbacks. The way to reap these benefits at scale, is to make testing it as non-intrusive and effortless as possible, and then do it in all callback-based tests by default, increasing the chances that it pays back.

This is all checked automatically by assert.step(), without any of the boilerplate! 🙂

Examples and more info at https://qunitjs.com/api/assert/verifySteps/