feat(rules): no-standalone-expect

[rabelfish]

This rule prevents expects that are not inside of test block or helper function as these expects will not execute.

[G-Rath]

Awesome! Someone of more authority will review this soon as well, but I've reviewed the implementation, and left a few comments.

Don't forget to run prettier on your files :)

[SimenB]

@G-Rath's review seems to cover all of my points :)

[rabelfish]

Hey @SimenB @G-Rath Thanks for the feedback! I think I fixed everything you guys mentioned. Let me know how it looks now!

[G-Rath]

@rabelfish no problem, thanks for the rule!

TypeScript is my primary wheelhouse these days, so feel free to ping me if you have any questions or want a second pair of eyes on anything :)

The main thing that's left is to add a couple more tests to help document the rules behaviour.

[benmonro]

resolves #342 Thanks @rabelfish for implementing this!

[rabelfish]

@SimenB I seem to be failing the ci because every time I make a commit I get this:

✖ message may not be empty [subject-empty]
✖ type may not be empty [type-empty]
✖ found 2 problems, 0 warnings

Can you explain to me what I need to do to resolve this?

[SimenB]

It wants you to make conventional (semantic) commits. See https://www.conventionalcommits.org/

If you want them to be green, a quick chore: *my thing* works. Otherwise, don't worry about it as I'll squash merge and fix the commit message when doing so 🙂 As long as linting and tests are green on CI, it's fine

[SimenB]

Left a couple of nits, this looks really good!

(I'm happy to address the nits myself if you don't wanna, just let me know 🙂)

[SimenB]

This is a really awesome contribution, thanks @rabelfish! And thanks for being patient with me 😛

[SimenB]

Oh, missing mention in the main readme. I'll fix it in 5 and merge

[rabelfish]

@SimenB Ah, I think I assumed isTestCase in utils would catch an each as well. should be easy to fix. let me know if you see anything else

[SimenB]

Yeah it should, but we (by that I mean @G-Rath 😀) haven't gotten to dealing with .each yet (#336 is about describe, but the same could be said about test/it.each).

That's the only issue I saw, but I didn't check all the violations. Happy to check again once we deal with .each 🙂 FWIW, I suppose we don't deal with .only and .skip either

[rabelfish]

Hmm so it actually looks like the utility functions work fine but the way the ast tree traverses the each, it actually exits the each call expression before getting to the expect. So the each is added onto my call stack, it is just popped off before it traverses into the callback. which is unexpected. The it.only and .skip work just fine

[rabelfish]

@SimenB @G-Rath okay so it the it.each is working but I am having some typescript trouble. If one of you could help me with that I'd super appreciate it 😃

[G-Rath]

I'm currently away from my computer right now, so while I can still help Ill be doing so from memory.

In regards to the utils: not entirely sure what you mean, but also haven't been following the last couple of bumps on this PR.

If it helps, the ast structure is such that the expect is always at the bottom: the topmost node is typically the right most "thingy".

At some point I want to have a talk about what we consider what, in terms of matching for rules - i.e ".toBe" (member expression) vs .toBe() (call expression)" - some rules you definitely want to match the CE, but others we could consider either legal but that's a talk for after we've finished converting.

I am in the process of rewriting the expect utility matchers, so that they're now doing a parsing thing that returns a handy object like this:

interface Expectation<
  Modifier extends ExpectModifier = ExpectModifier,
  Matcher extends string = string,
>{
  expect: ExpectCall;
  modifier?: ExpectMember<Modifier>;
  matcher?: ExpectMember<Matcher>;
}

I've actually finished these, but still in the process of converting the remaining rules - if I get the time I should have them done by this weekend.

But that won't help you now, so I'll take a look at your PR 😂

Feel free to dump the TS output here if you'd like.

Also, on mobile so sorry about mistakes in this comment

[SimenB]

@rabelfish I fixed the type error. Tested again on Jest's code base, and found another false positive. I added a failing test 🙂 We're getting really close!

[benmonro]

@SimenB FYI looks like @rabelfish fixed it, looks like build fails are just git related, we good to merge? we're doing a dog and pony show later today and would love to have @rabelfish show this off. :)

[SimenB]

we good to merge?

yeah, I was literally just verifying there were no false positives in Jest's code base

[benmonro]

cool, thanks!

[SimenB]

Thank you so much @rabelfish, this is a fantastic new rule. Thanks for being patient with me and working through the review!

[SimenB]

🎉 This PR is included in version 22.14.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[rabelfish]

@SimenB Thank you so much!! I appreciate the thoroughness 😄

[swissspidy]

Not sure if I'm wrong, but this doesn't seem to work well with jest-each.

I'm using it like this as described in the project's documentation:

each([[1, 1, 2], [1, 2, 3], [2, 1, 3]]).test(
  'returns the result of adding %d to %d',
  (a, b, expected) => {
    expect(a + b).toBe(expected);
  },
);

And now get an error caused by this rule.

The tests in this PR seem to cover the it.each() case, but not the each().it() case.

Please let me know if I'm doing something wrong, thanks.

[SimenB]

Huh, I didn't consider extensions when testing this. Could you open up a new issue?

Also, any reason why you use jest-each? With jest 23 it's built-in via test.each etc

[G-Rath]

@swissspidy @SimenB I've made #354 to cover this - I think this rule should follow the pattern of expect-expect with it's assertFunctionNames, as much as possible (and within reason) for the simple fact that this is Javascript - there's any number of interesting patterns people could use.

Once I've finished the new expect guards, and converted the last of the rules, I plan to swing back around and do the same re-implementation for test & describe blocks, which'll hopefully cover this as well.

[swissspidy]

Thanks!

In the meantime I‘ll see whether I can drop jest-each :-)

[SimenB]

Unless you're stuck on Jest 22 there's no reason to keep using jest-each directly - the module is part of the jest monorepo and is baked into the globals jest exposes