Handle arrow function test with except-simple configuration of the require-expect rule

[bmish]

Fixes #238.

This bug only affected the except-simple (default) configuration of the require-expect rule. The arrow function body of the test was incorrectly being detected as a greater nesting depth, preventing the test from detecting the top-level expect statement.

I also added test cases for arrow function tests to the other configurations of this rule.

CC: @raybaker

[coveralls]

Coverage remained the same at 100.0% when pulling 1744356 on bmish:require-expect-arrow-except-simple into ef8f129 on platinumazure:master.

[platinumazure]

Thanks @bmish for the PR (and @raybaker for reporting the original issue).

Changes look good at a glance, but I want to test the changes locally and understand better which of the added tests would have failed (this will also help me to make sure no test cases are missing). I will review tonight, hopefully.

[raybaker]

That was an incredibly quick response/fix @bmish / @platinumazure, thank you both!

[bmish]

This is the only test case that was broken before since this bug only affected the except-simple config. I added corresponding test cases to the other configs regardless.

[platinumazure]

I knew I had a good reason to be suspicious of this bug and the proposed solution... (Not due to bad code on your side or anything)

It looks like the real issue might be improper esquery syntax in the handlers which basically prevent ArrowFunctionExpressions (with body.type=BlockStatement) from being properly ignored by the handler. Not sure why that didn't throw, or emit warnings, or anything. But yeah, the original design definitely accounted for arrow functions and that's why I wanted to take a closer look at this one. (And you don't want to know how long I spent in node inspect until I finally noticed the issue...)

Would you mind trying the changes I suggest in this review, and see if the tests still pass accordingly? My thinking is, it shouldn't matter if we are counting the test function itself as a block depth, because the isViolatingExceptSimpleRule function is checking for blockDepth greater than 1. So basically, we're building the test itself as an acceptable block depth.

Another test you could add, to see if I'm on the right track here, would be an invalid case that has 2 nested blocks or functions (3 total blocks when you include the test function itself). If I'm right, this new test would pass with my proposed approach, but fail with your approach (because blockDepth would only be 1 due to the logic you're introducing).

Let me know if I'm not making sense. Thanks again for the effort so far and I appreciate your patience as I fit this review in while also moving out of my house to avoid re-roofing, and other disruptions to my life 😄

[bmish]

@platinumazure thanks for all the details and figuring out the better fix. I switched to your fix. However, I was unable to reproduce any test cases that your fix works for but mine doesn't.

[platinumazure]

Thanks for the changes. Guess I was wrong about the edge case unit test, but I'm not going to worry about it. I'll merge ASAP.

[bmish]

@platinumazure thanks! Hopefully I covered all the test cases although not 100% sure of course.

Probably a good time for a patch release of this along with the Github releases (#237).

[bmish]

@platinumazure also I'll plan to investigate why ESLint didn't throw an error with this invalid esquery syntax. That sounds like a breaking change to make if possible in the next version if ESLint to catch bugs like this.