Skip to content

Conversation

@Pixel998
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

  • Improved and completed the RuleTester TypeScript typings
  • Updated the type tests accordingly.

Is there anything you'd like reviewers to focus on?

@Pixel998 Pixel998 requested a review from a team as a code owner September 11, 2025 11:02
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Sep 11, 2025
@netlify
Copy link

netlify bot commented Sep 11, 2025

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 064bccd
🔍 Latest deploy log https://app.netlify.com/projects/docs-eslint/deploys/692c55905808080008ae90eb

@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Sep 11, 2025
@DMartens DMartens added the types Related to TypeScript types label Sep 11, 2025
@fasttime fasttime moved this from Needs Triage to Implementing in Triage Sep 11, 2025
@fasttime fasttime added accepted There is consensus among the team that this change meets the criteria for inclusion contributor pool labels Sep 11, 2025
@fasttime fasttime moved this from Implementing to Feedback Needed in Triage Sep 30, 2025
@nzakas nzakas added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Oct 31, 2025
@lumirlumir
Copy link
Member

There is now a merge conflict.

@fasttime
Copy link
Member

In the 2025-11-13 TSC Meeting, it was decided to add type definitions and documentation for the static RuleTester methods:

eslint/lib/types/index.d.ts

Lines 2236 to 2238 in d5fc247

static setDefaultConfig(config: Linter.Config): void;
static getDefaultConfig(): Linter.Config;
static resetDefaultConfig(): void;

@fasttime fasttime removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Nov 17, 2025
@fasttime fasttime moved this from Feedback Needed to Implementing in Triage Nov 17, 2025
fasttime
fasttime previously approved these changes Nov 19, 2025
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! Leaving open for @DMartens to verify.

@fasttime fasttime moved this from Implementing to Second Review Needed in Triage Nov 19, 2025
export namespace RuleTester {
interface ValidTestCase {
interface ValidTestCase
extends Omit<Linter.Config, "name" | "basePath" | "files" | "ignores"> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The following Linter.Config properties maybe should also be excluded:

  • linterOptions: this would only make sense for rules for inline configuration
  • plugins: only if we want to address a language or processor by its name instead of passing it in directly
  • rules: we want to test the rule in isolation (an edge case would be rules which have a side-effect like context.markVariableAsUsed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m fine with excluding linterOptions, plugins, and rules as well, but would like @fasttime’s opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

All config object keys can be also used in test cases. Excluding some of them from the typings is more a philosophical matter than a technical one. Now, linterOptions, plugins, and rules are not currently included in ValidTestCase, so I think we shouldn't add them if the team has any reservations. That said, I'd be in favor of including them if users request them for a specific use case at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Recently we decided to make the typings of data stricter in eslint/rewrite#310 even though other values (e.g. objects) can be stringified without error and there is no runtime check for this.
IIRC these properties were not allowed to be set in test cases before the introduction of the "flat" config.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC these properties were not allowed to be set in test cases before the introduction of the "flat" config.

linterOptions is a new option in flat config, so it was not allowed in test cases with the old RuleTester in ESLint v8. By contrast, plugins was allowed as an array of strings, and rules, in its current format, was already allowed prior to flat config. I checked the v8 code, and it looks like RuleTester has always worked the same way: moving all non-test-case properties to a config object, and using that config (after validation) to lint the provided code:

/*
* Assumes everything on the item is a config except for the
* parameters used by this tester
*/
const itemConfig = { ...item };
for (const parameter of RuleTesterParameters) {
delete itemConfig[parameter];
}
/*
* Create the config object from the tester config and this item
* specific configurations.
*/
config = merge(
config,
itemConfig
);

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. I think I thought that because I never saw those properties in rule test files.

@nzakas
Copy link
Member

nzakas commented Dec 9, 2025

@fasttime @DMartens what's the status on this?

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

@DMartens
Copy link
Contributor

DMartens commented Dec 9, 2025

The open question was whether we want to include uncommon configuration options like plugins for test cases as they are currently allowed.
@Pixel998 Changes LGTM, thank you.

@DMartens DMartens merged commit e7673ae into eslint:main Dec 9, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly types Related to TypeScript types

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

8 participants