search

dangreenisrael / eslint-plugin-jest-formatting

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

Fix some weirdness with expression => expect => expression => expect #51

Closed benkimpel closed 5 years ago

benkimpel commented 5 years ago

Description

If there are multiple expects intermixed with expressions in a test block things can get a little weird.

Example:

test('it does something', () => {
  expressionA();
  expect(true).toEqual(true);
  expressionB();
  expressionC();
  expect(true).toEqual(true);
})

Becomes...

test('it does something', () => {
  expressionA();

  expect(true).toEqual(true);
  expressionB();
  expressionC();

  expect(true).toEqual(true);
})

When ideally it would be...

test('it does something', () => {
  expressionA();

  expect(true).toEqual(true);

  expressionB();
  expressionC();

  expect(true).toEqual(true);
})

Proposal

I think the fix might just simply be changing the config for padding-before-expect in index.ts to...

[
    { blankLine: 'always', prev: '*', next: 'expect' },
    { blankLine: 'always', prev: 'expect', next: '*' },
    { blankLine: 'any', prev: 'expect', next: 'expect' },
]

As well as adding the same addition to the padding-before-all rule.

If my guess is correct, then the fix will just be tossing that in and writing some specs. I could be wrong, though... I haven't tested the suggestion at all.

Update, we are going to update all the rules to match this pattern of around

dangreenisrael commented 5 years ago

👍

benkimpel commented 5 years ago

One left!!

benkimpel commented 5 years ago

@dangreenisrael I wanted to check with you on a small inconsistency I noticed.

We have... test/lib/rules/padding-around-expect-groups.spec.js docs/rules/padding-around-expect-statements.md and in the README.md padding-around-expect-statements

I think you had wanted to go with expect-groups, but lmk.

dangreenisrael commented 5 years ago

yes, it did. Nice catch @benkimpel. Thanks!!!

it should be expect-groups

dangreenisrael commented 5 years ago

I'm also open to other suggestions.

benkimpel commented 5 years ago

No preference here! I'll change it to groups in a PR tomorrow and I have a PR almost ready for the last bit of this (around test/it).

benkimpel commented 5 years ago

@dangreenisrael I'm going to close this and leave discussion for the padding-around-all rule discussion in #50. I came across a few other options for configuring test regexes worth considering and if they work out then the main benefit of configuring a single rule.

I'm also going to open up an issue later for cleaning up padding.ts. I don't think we're going to expose that base rule directly, so there's a lot of clean up i can do in makeRule and padding.ts.

LMK if you have any concerns or objections.

benkimpel commented 5 years ago

Hm. Actually we missed a few...

fit fdescribe ftest (can't tell if this one is real, might just be test.only) xit xdescribe xtest

benkimpel commented 5 years ago

I can take care of those along with padding-around-all ^^

dangreenisrael commented 5 years ago

Awesome thanks. We should probably also add test cases for test.only, it.only and describe.only as well as test.skip, it.skip, and describe.skip

benkimpel commented 5 years ago

@dangreenisrael if #70 is merged then I'll open a PR for https://github.com/dangreenisrael/eslint-plugin-jest-formatting/tree/51/missing-tokens-and-padding-around-all which is based on the work in #70. That will be the final PR on this issue.