Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@typescript-eslint/internal/plugin-test-formatting too strict about snapshot indentation #4911

Closed
Josh-Cena opened this issue May 5, 2022 · 3 comments · Fixed by #5379
Closed
Labels
accepting prs Go ahead, send a pull request that resolves this issue package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin tests anything to do with testing

Comments

@Josh-Cena
Copy link
Member

Complained in #4901 (comment)

Take this code for example (from that PR):

{
      code: `
class Data {
  member<T extends unknown>() {}
}
      `,
      errors: [
        {
          data: { constraint: 'unknown', name: 'T' },
          messageId: 'unnecessaryConstraint',
          endColumn: 27,
          column: 10,
          line: 3,
          suggestions: [
            {
              messageId: 'removeUnnecessaryConstraint',
              output: `
class Data {
  member<T>() {}
}
              `,
            },
          ],
        },
      ],
    },

The test fails, suggesting that:

Expected value to strictly be equal to:
      "
    class Data {
      member<T>() {}
    }
                  "
    Received:
      "
    class Data {
      member<T>() {}
    }
          "

Okay, let's try another way by indenting everything—maybe it will be dedented? (Spoiler: I don't know how the snapshot works.)

{
      code: `
        class Data {
          member<T extends unknown>() {}
        }
      `,
      errors: [
        {
          data: { constraint: 'unknown', name: 'T' },
          messageId: 'unnecessaryConstraint',
          endColumn: 35,
          column: 18,
          line: 3,
          suggestions: [
            {
              messageId: 'removeUnnecessaryConstraint',
              output: `
                class Data {
                  member<T>() {}
                }
              `,
            },
          ],
        },
      ],
    },
    - Expected
    + Received


    -                 class Data {
    -                   member<T>() {}
    -                 }
    -               
    +         class Data {
    +           member<T>() {}
    +         }
    +       

o_o

It turns out the only way to work around it is by either fighting the linter and adding ignore comments, or by using .trimEnd() to each snapshot.

I think it's fine for the snapshots to be formatted nicely, so what about a sanitation step that removes all extraneous indentation, or removes trailing spaces (like .trimEnd())? I think ESLint's tests does that.

@Josh-Cena Josh-Cena added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 5, 2022
@bradzacher
Copy link
Member

yeah in general we just add trimEnd to these cases.
The difficult thing about adding trimEnd at a framework level is that we don't know if that whitespace actually matters.
For example - what happens if a fixer intentionally fixes const x = 1; to const x = 1; ? Then it would be impossible to test the fixer!

@Josh-Cena
Copy link
Member Author

what happens if a fixer intentionally fixes const x = 1; to const x = 1; ?

It shouldn't matter, though. That should fall under ESLint's principle of "fixers shouldn't care about code style". At worst it would mean the user needs to format their code again after fixing, which they probably always do anyways.

@JoshuaKGoldberg
Copy link
Member

Following up on this issue: I agree, it's annoying to have to battle with Prettier on test output properties. If a fixer does create some bad whitespace situation, we'll see it explicitly in the test file.

@JoshuaKGoldberg JoshuaKGoldberg added tests anything to do with testing accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Jun 25, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin tests anything to do with testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants