[prefer-expect-assertions] Addition of an "ifPromisesUsed" option

[erunion]

I really like the prefer-expect-assertions rule, but requiring a test file to be covered in either expect.assertions(n) or expect.hasAssertions() calls when assertions aren't nested inside promises gets a little messy.

It would be rad to be able to have an ifPromisesUsed option on the rule that when enabled, would only throw the rule if:

• The test case returns a Promise directly to Jest and
• There are no expect() calls inside of the Promises resolve or reject methods.

test('should not trigger the rule because there is explicit expectation by itself', () => {
  expect(true).toBeTruthy();
});

test('should trigger the rule because there is an expectation inside of a Promise resolver', () => {
  expect.hasAssertions(); // expect.assertions(1);
  return methodThatShouldAPromise(done => {
    expect(true).toBeTruthy();
    done();
  });
});

[SimenB]

There are 2 use cases for expect.hasAssertions(); - one is the promises one you point out, and the other is conditional logic (if or throw). All these cases are better covered by https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/valid-expect-in-promise.md, https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-try-expect.md and https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-if.md. If you use those rules, you won't need prefer-expect-assertions as the cases it tries to guard against should be covered.

---

That said, I'm not against the option you're suggesting. @G-Rath thoughts?

[G-Rath]

I'm in a similar boat as you @SimenB - I've actually typed this out a couple of times as I ran through & ruled out the possible advantages.

I think you've hit the nail w/ valid-expect-in-promise - by having that enabled, it reduces this rule to either all or nothing; b/c otherwise what's the reasoning for not having it in the first test?

i.e You want it in all promise/callback tests, b/c they could be more easily broken via refactoring; but that concern is already covered by the rules you mentioned.

Hence I don't see a major use for this, especially vs the possible issues we could hit in trying to ensure detection of methods that are promise resolvers.

---

That said, I'm not against reviewing an PR showing the advantage over valid-expect-in-promise; can't promise it'll be merged if we don't think it adds enough.

[erunion]

valid-expect-in-promise, no-try-expect, and no-if certainly help to ensure that an expect is present in a test, but the main differentiation I see with prefer-expect-assertions is that it helps to ensure that you're writing assertions that are going to be run.

Jest will gladly run a test case where no assertions are called because something deep in the code that you're testing is triggering some case, causing those assertions to not be run (like in the case of a promise not resolving or rejecting as it should). By requiring expect.hasAssertions or expect.assertions(n), you can tell Jest "hey, something is supposed to happen here, so yell at me if it doesn't.".

And I think by allowing prefer-expect-assertions to be conditionally applied, in addition to the three other rules already mentioned, people can get new tooling to assist in rooting out bugged code from codebases without the burden of writing expect.hasAssertions boilerplate in every test.

I'm happy to work on a PR.

[G-Rath]

@erunion

re the burden of writing expect.hasAssertions boilerplate, you can use a setupFilesAfterEnv script:

/**
 * Sets up before each test an expectation for there to be assertions.
 */
beforeEach(() => expect.hasAssertions());

https://github.com/G-Rath/babel-plugin-replace-ts-export-assignment/blob/master/test/setupExpectEachTestHasAssertions.ts

https://github.com/G-Rath/babel-plugin-replace-ts-export-assignment/blob/8dfdca32c8aa428574b0cae341444fc5822f2dc6/package.json#L52-L54

Granted you can't use that if you're using tools other than expect, but that'll do a lot of what you want, and you can invert it by adding expect(true).toBe(true) at the top of tests that would otherwise fail, if you wanted.

---

I think by allowing prefer-expect-assertions to be conditionally applied

I completely agree, but it's the conditions that I'm not convinced we can accurately match enough of w/o edge cases and generally ending up attempting to do the job of a static analyzer - that's why it's generally best practice to always stick expect.hasAssertions in your tests if possible, regardless of complexity.

The focus of my review will be the complexity of the implementation vs what it actually covers, and what it doesn't cover (i.e "if you import methodThatShouldAPromise then this option bails out").

None of this is meant to discourage you from making a PR if you want to have a go; I just want to share some of my thoughts & concerns 🙂

[jtheberge]

What are thoughts on option for detecting async it/test functions? With no-test-return-statement enabled, no promises would be returned so looking for async and applying in scope of func should guarantee it's conditional on any async tests only

[G-Rath]

I'm pretty sure this was sorted via #677, so going to close this :)