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

feat: Hooks for test cases #122

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

RobinTail
Copy link

@RobinTail RobinTail commented Aug 13, 2024

Summary

I propose adding an optional setup before (and after) properties to the test cases that the ESLint RuleTester runs. This
feature will allow developers to prepare specific environments needed for certain rules before running each test case.

Related Issues

@eslint-github-bot
Copy link

Hi @RobinTail!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@RobinTail RobinTail changed the title Hooks for test cases feat: Hooks for test cases Aug 13, 2024
@RobinTail RobinTail marked this pull request as ready for review August 13, 2024 20:13
@fasttime fasttime added the Initial Commenting This RFC is in the initial feedback stage label Aug 14, 2024
designs/2024-hooks-for-test-cases/README.md Show resolved Hide resolved
designs/2024-hooks-for-test-cases/README.md Outdated Show resolved Hide resolved
designs/2024-hooks-for-test-cases/README.md Outdated Show resolved Hide resolved
designs/2024-hooks-for-test-cases/README.md Outdated Show resolved Hide resolved
@jfmengels
Copy link

Do you think setup will in practice only be used to set up test files? I can imagine it also being used for environment variables (though I don't know if any rules do that in practice), but I'm failing to see other use-cases.

Some prior art on the subject: elm-review allows looking at multiple files, including the elm.json file (the equivalent of package.json, example). Because this is supported out of the box - and rules have no means to do side-effects to load files like is done by no-extraneous-dependencies (my baby from 8 years ago, time flies) - there is a way in tests to construct a mock project (example) and to use that in a test (example).

I think that if ESLint ever adds native support for reading package.json files or for multi-file analysis, then an test API dedicated for setting the value of files will be preferable.

In the meantime, I guess that a setup function property does the trick, but as mentioned, it can be misused and can lead to interdependent tests.

@RobinTail
Copy link
Author

Do you think setup will in practice only be used to set up test files?

I don't.

@jfmengels
Copy link

@RobinTail What else do you think it will or could be used for in practice?

@RobinTail
Copy link
Author

RobinTail commented Aug 18, 2024

I know what I need it for, but humans in general are very inventive beings, @jfmengels , some of them find very unusual applications for quite ordinary items.

image

@RobinTail
Copy link
Author

  • setup renamed to before
  • added after
  • mentioned the duplicates detection drawback

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just few more details to clarify.

designs/2024-hooks-for-test-cases/README.md Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Sep 11, 2024

@RobinTail there is some other feedback to address. Please take a look.

@RobinTail
Copy link
Author

yeah, sorry for delay — something came up and I'll get back to this soon.
thanks for reminder, @nzakas

@RobinTail
Copy link
Author

Addressed, @nzakas

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Moving to final commenting.

@nzakas nzakas added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Sep 12, 2024
Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants