dangreenisrael/eslint-plugin-jest-formatting

ESLint rules for formatting test suites written for jest.

eslint-plugin-jest-formatting's Issues

[BUG] undocumented rule: (padding-around-all)

The following rule is undocumented: https://github.com/dangreenisrael/eslint-plugin-jest-formatting/blob/master/src/index.ts#L133.

I would expect it to be mentioned on the readme, and linked back to a .md file that simply lists all the other rules that it controls. I'll leave it up to you the maintainers of this project as to whether those rules that padding-around-all controls also make mention that they can be configured in this way.

[Rule] Add padding around describe blocks

Explanation of the rule
Ensure that describe blocks have exactly one line of spacing between them.

Valid code example

describe('foo', ()=>{
  test('foo bar', ()={
    //...
  })
}

describe('bar', ()=>{
  test('bar foo', ()={
    //...
  })
}

Invalid code example

describe('foo', ()=>{
  test('foo bar', ()={
    //...
  })
}
describe('bar', ()=>{
  test('bar foo', ()={
    //...
  })
}

Node engines is very specific

Hello, @dangreenisrael. Thanks for writing this plugin. It's exactly what I was looking for to correct some questionable style preferences from a certain member of my team. 😆 😉

It seems like the engines.node value in package.json is perhaps overly specific. Any reason not to use >= 8.10.0?

When installing I had to use yarn add --dev --ignore-engines eslint-plugin-jest-formatting. (I'm on 8.16.0) First time I've ever had to ignore-engines.

This isn't really a bug or anything. Just a question and a thank you.

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

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

[BUG] .nvmrc file needs an update

Describe the bug

The .nvmrc file currently states v8.10.0 but this causes the following error when running this project.

error [email protected]: 
The engine "node" is incompatible with this module. 

Expected version "^10.12.0 || >=12.0.0". Got "8.10.0"
error Found incompatible module.

The fix is simple. Fix the node version in .nvmrc. But which version should be specified? 🤔

To Reproduce
Steps to reproduce the behavior:

1. clone this repo
2. run yarn

Expected behavior
No error should happen when you run yarn.

[Rule Update] padding-around-expect-groups configuration/default

Hi! Love the plugin, great work on this 🎉

When we turned on the plugin:jest-formatting/recommended preset recently I was surprised by the behaviour of padding-around-expect-groups. We have a test that looks like this:

test("should return true from the correct state", () => {
  const loggedInState = { id: 1, loggedIn: true };
  expect(isUserLoggedIn(loggedInState)).toBe(true);
});

When we ran the recommended rules on this code, it suggested it should instead look like this:

test("should return true from the correct state", () => {
  const loggedInState = { id: 1, loggedIn: true };

  expect(isUserLoggedIn(loggedInState)).toBe(true);
});

In this case, I disagree with the linter because I think that the extra line of whitespace actually makes this code less legible than before. It reads more like there was something meant to go between those lines that was dropped for some reason. I found myself thinking that if I hit this lint error while writing the code, that I would be tempted to write it into the following one-liner and losing the value of the named variable:

test("should return true from the correct state", () => {
  expect(isUserLoggedIn({ id: 1, loggedIn: true})).toBe(true);
});

Would it be possible to add configuration options to this rule for something like a threshold where it didn't re-write blocks if it added "too much" whitespace? Alternatively, would you consider leaving this out of the "recommended" preset?

[DOCS] Links are wrong in `docs/rules/padding-before-all.md`

The links are wrong in the padding-before-all.md docs page.

https://github.com/dangreenisrael/eslint-plugin-jest-formatting/blob/master/docs/rules/padding-before-all.md

Blank line before expect

Blank line before expect or block of expects

Valid code example:

  it("test name", () => {
    const someVariable = "...";

    expect(someVariable).toBe(2);
    expect(someVariable).toBe(1);
  })

Invalid code example:

    it('test name', () => {
        const someVariable = '...';
        expect(someVariable).toBe('');
        expect(someVariable).toBe('');
    });

[BUG] GitHub releases are maybe broken

Describe the bug

Not sure what is causing this, but GitHub release page seems to be broken.

Doesn't have the descriptions.

And also, the last 3 releases are folded, and don't immediately stand out.

To Reproduce

Visit release page: https://github.com/dangreenisrael/eslint-plugin-jest-formatting/releases

Expected behavior

Each release has description and correct version.

Screenshots

Desktop (please complete the following information):

macOS

Additional context

N/A

[Rule] Newline before expect

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.)

🤙 🤝 Happy to do a PR if you're interested. 🍰 👍

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);
});

Support ESLint 8.x

ESLint v8.0.0 is released 🎉

It would be awesome to have official ESLint 8 support. 👊
I'm happy to help where I can of course 🙂

[BUG] Property "overrides" is the wrong type

Describe the bug

After upgrading to 1.1.1, getting the following error:

Error: ESLint configuration in .eslintrc.js » plugin:jest-formatting/strict is invalid:
	- Property "overrides" is the wrong type (expected array but got `{"files":["**/*.test.*","**/*_test.*","**/*Test.*","**/*.spec.*","**/*_spec.*","**/*Spec.*","**/__tests__/*"],"rules":{"jest-formatting/padding-around-all":2}}`).

To Reproduce

Use 1.1.1 packaged as described in the docs.

Expected behavior

No errors.

Screenshots

N/A

Desktop (please complete the following information):

macOS

Additional context

N/A

[BUG] Rules not properly marked as deprecated

Eslint has an explicit mechanism for marking a rule as deprecated: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint/index.d.ts#L295.

I maintain eslint-config-intense which uses eslint-find-rules during upgrades of configs and plugins. During a usage I noticed all the previous rules which are deprecated have not been marked as such.

NOTE: Deprecated rules are found by looking at the metadata of the rule definition. All core rules and many plugin rules use this flag to indicate deprecated rules. But if you find a plugin that does not mark their rules as deprecated in the rule metadata, please file a pull request with that project.

• eslint-find-rules readme.md

Support ESLint 7.x

ESLint v7.0.0 is released 🎉

It would be awesome to have official ESLint 7 support. 👊
I'm happy to help where I can of course 🙂

[BUG] TypeError when extend recommended config

Describe the bug
eslint throws TypeError: Cannot read property 'recommended' of undefined when extend recommended config

TypeError: Cannot read property 'recommended' of undefined
Referenced from: /Users/dmitrijk/Projects/orders-manager-frontend/.eslintrc.json
    at loadConfigFile (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:216:40)
    at loadFromDisk (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:523:18)
    at load (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:587:20)
    at configExtends.reduceRight (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:453:36)
    at Array.reduceRight (<anonymous>)
    at applyExtends (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:431:26)
    at loadFromDisk (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:551:22)
    at Object.load (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config/config-file.js:587:20)
    at Config.getLocalConfigHierarchy (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config.js:240:44)
    at Config.getConfigHierarchy (/Users/dmitrijk/Projects/orders-manager-frontend/node_modules/eslint/lib/config.js:192:43)

To Reproduce
have following devDependencies in package.json:

{
    "eslint": "~5.16.0",
    "eslint-config-airbnb": "^17.1.1",
    "eslint-plugin-import": "^2.18.2",
    "eslint-plugin-jest-formatting": "^0.1.0",
   ...
}

have following .eslintrc.json:

{
  "env": {
    "browser": true,
    "es6": true
  },
  "extends": [
    "eslint:recommended",
    "airbnb",
    "plugin:jest-formatting/recommended"
  ],
  "globals": {
    "Atomics": "readonly",
    "SharedArrayBuffer": "readonly"
  },
  "parserOptions": {
    "ecmaFeatures": {
      "jsx": true
    },
    "ecmaVersion": 2018,
    "sourceType": "module"
  },
  "plugins": ["jest-formatting"],
  "rules":
  ...
  }
}

Of course I have installed plugin via yarn add eslint-plugin-jest-formatting --dev.
I have node_modules/eslint-plugin-jest-formatting/lib/index.js file. Exactly this file causes this issue.

Expected behavior
No error happens

Desktop (please complete the following information):

• OS: MacOS Mojave v10.14.
• Node: v10.16.0
• Yarn: 1.17.3

[Rule] test name should be trimed

I don't like when people leave white spaces at the beginning and end of tests. It's weird.

Valid code example

it("is a valid test name", () => {
  // code
})

Invalid code example

it(" is not a valid test name", () => {
  // code
})

it("is not valid either ", () => {
  // code
})

Allow configurable test file pattern

Description

The plugin currently identifies a Jest file by checking the filepath for the string "test" or "spec". This probably covers most real world usage, but it could also result in some false positives or false negatives.

False Positives

With false positives the impact is low. It's unlikely that the false positive is also using functions matching Jest names so it will probably just result in wasted time during a lint run.

• src/inspectors/RequestInspector.js
• src/contestants/ContestantProfile.jsx
• test/utils/justSomeUtilFile.js

False Negatives

False negatives will be high impact, because they're the target of the plugin and they're missed.

• src/components/Thing/ThingTest.jsx

  Case issue + some people keep the test next to the component and not in a test directory.

• src/components/Thing/__pruebas__/thing.js

  Someone has customized their Jest set up.

Proposal

Jest supports a configuration option called testRegex that let's one customize how test files are matched.

It makes sense for this plugin to match its default to the Jest default. It will also be important to allow configuration of the testRegex in this plugin in case any users have configured their Jest to something different.

A possibly clever or possibly terrible solution could be to see if we could read the Jest config instead of duplicating the configuration ability. <= Nah... too brittle.

[Rule] Disable padding around todo test blocks

There should not be padding around todo test blocks.

Valid code example

describe("valid", () => {
  it.todo("foo")
  it.todo("bar")
  it.todo("baz")
})

Invalid code example

describe("invalid", () => {
  it.todo("foo")
  
  it.todo("bar")
  
  it.todo("baz")
})

Maybe this can be an option to padding-around-test-blocks ?

[Rule] Newline after setup and teardown

Explanation of the rule
This rule would enforce a new line after setup and teardown blocks.

Note: these examples come from the jest docs

Valid code example

describe('some tests', () => {
  beforeEach(() => {
    initializeCityDatabase();
  });

  afterEach(() => {
    clearCityDatabase();
  });

  test('city database has Vienna', () => {
    expect(isCity('Vienna')).toBeTruthy();
  });
})

Invalid code example

describe('some tests', () => {
  beforeEach(() => {
    initializeCityDatabase();
  });
  afterEach(() => {
    clearCityDatabase();
  });
  test('city database has Vienna', () => {
    expect(isCity('Vienna')).toBeTruthy();
  });
})

[BUG] `padding-around-expect-groups` wants space around `expect`s which are `await`ed

Describe the bug
If I have two expects right next to each other, and one of them is awaited, the padding-around-expect-groups rule wants there to be a blank line between them.

To Reproduce
Example code:

await expect(subject.asyncThingWhichThrows()).rejects.toThrow(SomeError);
expect(subject.thingWhichWorks()).toBe('some value');

Expected behavior
awaited expects should be allowed to be right next to regular ones.

Screenshots
Real example:

Desktop (please complete the following information):

• Windows 10
• Node 14
• ESLint 7.24
• plugin version 2.0.1

Support ESLint v9 (including Flat Config)

ESLint v8 already supports the new "flat" configuration format and in ESLint v9, it will become the default. It will still be possible to use this plugin then, but it'll require a legacy wrapper. It would be nice if this plugin could add native support for flat config.

Some helpful links:

• https://eslint.org/docs/next/use/configure/migration-guide
• eslint/eslint#18093
• jest-community/eslint-plugin-jest#1408

---

EDIT: ESLint v9 has been released now.

[BUG] `src/something/tests.tsx` doesnt get looked at

Describe the bug
We have files like src/something/tests.tsx and the rules of this repo don't run there!

Expected behavior
The rules should run on there too.

Additional context
I noticed that on https://github.com/dangreenisrael/eslint-plugin-jest-formatting/blob/master/src/index.ts#L141-L151 there is a list of filenames, and ours is not in there. I could make a PR to add these, but also I think we can probably remove the overrides bit from this file, and put the rules one level up, as a sibling to plugins, and everything will work well.

Normally (and we too) people have a testMatch on the jest config, so it already knows all the test files of the project. No need to have special detection on this repo.

Let me know if I should make a PR to add our test or if I should make a PR removing the overrides.

[BUG] Comments are dropped from before and after test blocks

Explanation of the problem

Comments are getting dropped

Input

test('foo', ()=>{})
/*
Comment
*/
describe('bar', ()=>{

})

Expected Output

test('foo', ()=>{})

/*
Comment
*/
describe('bar', ()=>{

})

Actual output

test('foo', ()=>{})

describe('bar', ()=>{

})