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

[ryanwilsonperkin]

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?

[hockeybuggy]

I am a big fan of using whitespace as a way to communicate different sections of the tests. On my best days I prefer:

it("should do that thing", () => {
// [setup]

// [preconditions]

// exercise subject of test

// postconditions

// [cleanup]
})

The think that I don't like with this rule is that it's assuming that the only thing in the preconditions and postconditions sections are expect calls. As shown in @ryanwilsonperkin's examples it's sometimes prudent to alias something by assigning to a variable.

I do think that this rule may be something that some people would like to have access to, but think that it should be excluded from the recommend preset.

[dangreenisrael]

@benkimpel thoughts?

[benkimpel]

Hey @ryanwilsonperkin and @hockeybuggy!

It's funny, but the expect behaviour we implemented is exactly what I needed for my team, but it's good to see some feedback on it in practice from people that I do not boss around. Let me share the thinking that went into it and then let me know if that changes any minds. ;^) It's ok if it doesn't, formatting is subjective and personal.

The goal was to support as best we can the Arrange Act Assert pattern of writing tests, wherein the blank line is an important part of delimiting the steps. This plugin can't get us all the way there by any means, but it can handle maybe 90% of the separation between act and assert. There are other ESLint rules (padding-line-between-statements which this rule is based on) that can add a padding line between variable declarations at the top of a scope so we can get closer still... but a full programmatic implementation of Arrange Act Assert is probably not possible. Still, 90% reliable separation between Act and Assert seemed like a win for me.

So that was the goal, but as noted above formatting is subjective. For an opinionated formatting plugin like this one the best we can do is pick something that most people like as the default and enforce it while providing a few escape hatches along the way. So long story short... It's all good with me to take it out of recommended if people feel strongly about it. I'd vote for removing it from recommended over making it a configuration option of some kind.

@ryanwilsonperkin as a workaround for now you'll want to list the rules individually, excluding the expect of course.

[dangreenisrael]

I'm +1 to taking this our of the recommended config. Especially since @benkimpel is working on an 'all' rule.