dangreenisrael / eslint-plugin-jest-formatting

ESLint rules for formatting test suites written for jest.
MIT License
156 stars 13 forks source link

[Rule] Newline before expect #41

Closed benkimpel closed 5 years ago

benkimpel commented 5 years ago

Explanation of the rule Add a newline before expect unless it's the only statement in the block or if it's preceded by another expect.

Provides better support for Arrange, Act, Assert style.

(One can already support newlines after arrange and act with ESLint's padding-line-between-statements rule.)

:call_me_hand: :handshake: Happy to do a PR if you're interested. :cake: :+1:

Valid code example

test("'yes' is truthy", () => {
  expect("yes").toBeTruthy();
});

test("array index", () => {
  const data = ["a", "b", "c"];

  expect(data[1]).toEqual("b");
});

test("object assignment", () => {
  const data = { one: 1 };

  data.two = 2;
  data['three'] = 3;

  expect(data).toEqual({ one: 1, two: 2, three: 3 });
  expect(data.three).toEqual(3);
});

Invalid code example

test("array index", () => {
  const data = ["a", "b", "c"];
  expect(data[1]).toEqual("b");
});

test("object assignment", () => {
  const data = { one: 1 };
  data.two = 2;
  data['three'] = 3;
  expect(data).toEqual({ one: 1, two: 2, three: 3 });
  expect(data.three).toEqual(3);
});
dangreenisrael commented 5 years ago

@benkimpel This is awesome. A PR would be more than welcome! +1

benkimpel commented 5 years ago

@dangreenisrael Cool. I'll get started soon.

benkimpel commented 5 years ago

Hey @dangreenisrael, I wanted to run something by you before I move forward...

As I started work on this I noticed that we could probably implement all of the Jest functions by basing our work off of the implementation of the rule that I mentioned above (padding-line-between-statements).

Here's the source and the docs. Take a peekaboo at that real quick... it's great stuff. There's lots of stuff that I wouldn't have considered, honestly.

I took that code and removed all of the StatementTypes (and associated helpers) except for the wildcard. Then I added StatementTypes for each of the Jest functions that we care about (beforeEach, beforeAll, expect, describe, etc.)... I had to inspect the tokens for this.

I have a rough proof of concept with this working, but it's a divergence from the existing work so I thought it'd be best to check-in and get your take on things. This approach should be able to support the current rules for describe and test as well as all the other stuff from Jest.

There are a few things I still need to consider, like would this rule fight with padding-line-between-statements or does ESLint handle this gracefully for rules that follow and change things from a previous rule. (I don't know yet.)

But here's the gist of how it would work. The rule config would follow the same approach as the ESLint rule.

// Newline before expect unless there's an expect before it
"rules": {
  "jest-formatting/padding": [
    "error",
    { "blankLine": "always", "prev": "*", "next": "expect" },
    { "blankLine": "never", "prev": "expect", "next": "expect" }
  ]
}
// Newline before all jest functions...
"rules": {
  "jest-formatting/padding": [
    "error",
    { 
      "blankLine": "always", 
      "prev": "*", 
      "next": ["beforeEach", "beforeAll", "expect", "describe", "test", "it", "afterEach", "afterAll"]
    },
    // You can further customize, because the last matching item wins
  ]
}

So, let me know your thoughts! I can finish the rough proof of concept and see where that gets us. We could work on thorough testing together. IDK. Up to you!

benkimpel commented 5 years ago

Again, I have no idea if this will pan out... but it's a good check-in point.

dangreenisrael commented 5 years ago

Sounds promising! Go for it!

dangreenisrael commented 5 years ago

@benkimpel One thing I would ask is that the API stays the same for users. i.e. I'm cool with also supporting the style of defining errors that is used by the block padding package, but I want to continue to support the existing style

 "rules": {
    "jest-formatting/padding-before-test-blocks": 2,
    "jest-formatting/padding-before-describe-blocks": 2
  }
benkimpel commented 5 years ago

Makes sense. I bet we could just build the "real" options structure internally.

benkimpel commented 5 years ago

@dangreenisrael Ready for a look... https://github.com/benkimpel/eslint-plugin-jest-formatting/commit/2bcd53168ff77e227a586ccdb27af2ee7bdbd442

It's still WIP, but check the commit message there for the details. You can also run it and see how it goes for you or implement some additional Jest functions following the new pattern and see how that feels.

benkimpel commented 5 years ago

It worked out pretty nicely so far.

benkimpel commented 5 years ago

Oh yeah! Also, there'd obviously be tests for padding.ts and some filepath filtering for detecting that it's a test.

benkimpel commented 5 years ago

LMK if you'd like to move forward

dangreenisrael commented 5 years ago

Can you open a PR against the main repo for the sake of discussion?

benkimpel commented 5 years ago

Sure. I'll clean it up a bit and open a more focused PR.

benkimpel commented 5 years ago

Closin' it!