Skip to content

chore(eslint-plugin): consistently use it in tests #10680

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

chore(eslint-plugin): consistently use it in tests #10680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/eslint-plugin/tests/areOptionsValid.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ const exampleRule = createRule<['value-a' | 'value-b'], never>({
name: 'my-example-rule',
});

test('returns true for valid options', () => {
it('returns true for valid options', () => {
expect(areOptionsValid(exampleRule, ['value-a'])).toBe(true);
});

describe('returns false for invalid options', () => {
test('bad enum value', () => {
it('bad enum value', () => {
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I actually like test() here over it(). The test name doesn't grammatically read out as a full sentence. I've personally kind of gravitated towards:

  • it(): for most tests, if there's at all a way to phrase it as "it X when Y" or something like that
  • test(): as a fallback if there's no logic, just a descriptor - like "test X"

Copy link
Contributor Author

@43081j 43081j Jan 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really care which one we use as long as it is one, and not two

If you can get the others to agree, I'm happy to update

I don't think you should end up with both. What you say makes sense but, if anything, suggests you would prefer having test throughout

Copy link
Member

Choose a reason for hiding this comment

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

Oh don't get me wrong, I think it() is the right choice almost all of the time. Just my personal nitpicky preference is to use test() in specific situations. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, my preference is actually suite/test/assert. But the most consistent thing we can do right now is it it seems

As a middle ground, id prefer to later move from the now-consistent it to test if anything

expect(areOptionsValid(exampleRule, ['value-c'])).toBe(false);
});

test('bad type', () => {
it('bad type', () => {
expect(areOptionsValid(exampleRule, [true])).toBe(false);
});
});
18 changes: 9 additions & 9 deletions packages/eslint-plugin/tests/docs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('Validating rule docs', () => {
const filePath = path.join(docsRoot, `${ruleName}.mdx`);
const { fullText, tokens } = parseMarkdownFile(filePath);

test(`${ruleName}.mdx must start with frontmatter description`, () => {
it(`${ruleName}.mdx must start with frontmatter description`, () => {
expect(tokens[0]).toMatchObject({
raw: '---\n',
type: 'hr',
Expand All @@ -235,7 +235,7 @@ describe('Validating rule docs', () => {
});
});

test(`${ruleName}.mdx must next have a blockquote directing to website`, () => {
it(`${ruleName}.mdx must next have a blockquote directing to website`, () => {
expect(tokens[4]).toMatchObject({
text: [
`🛑 This file is source code, not the primary documentation location! 🛑`,
Expand All @@ -247,7 +247,7 @@ describe('Validating rule docs', () => {
});
});

test(`headings must be title-cased`, () => {
it(`headings must be title-cased`, () => {
// Get all H2 headings objects as the other levels are variable by design.
const headings = tokens.filter(tokenIsH2);

Expand All @@ -269,7 +269,7 @@ describe('Validating rule docs', () => {
...requiredHeadings,
]);

test('important headings must be h2s', () => {
it('important headings must be h2s', () => {
for (const heading of headings) {
if (importantHeadings.has(heading.raw.replaceAll('#', '').trim())) {
expect(heading.depth).toBe(2);
Expand All @@ -278,7 +278,7 @@ describe('Validating rule docs', () => {
});

if (!rules[ruleName as keyof typeof rules].meta.docs?.extendsBaseRule) {
test('must include required headings', () => {
it('must include required headings', () => {
const headingTexts = new Set(
tokens.filter(tokenIsH2).map(token => token.text),
);
Expand Down Expand Up @@ -314,7 +314,7 @@ describe('Validating rule docs', () => {
for (const property of Object.keys(
schemaItem.properties as object,
)) {
test(property, () => {
it(property, () => {
const correspondingHeadingIndex =
headingsAfterOptions.findIndex(heading =>
heading.text.includes(`\`${property}\``),
Expand Down Expand Up @@ -359,7 +359,7 @@ describe('Validating rule docs', () => {
});
}

test('must include only valid code samples', () => {
it('must include only valid code samples', () => {
for (const token of tokens) {
if (token.type !== 'code') {
continue;
Expand All @@ -385,7 +385,7 @@ describe('Validating rule docs', () => {
}
});

test('code examples ESLint output', () => {
it('code examples ESLint output', () => {
// TypeScript can't infer type arguments unless we provide them explicitly
linter.defineRule<
keyof (typeof rule)['meta']['messages'],
Expand Down Expand Up @@ -528,7 +528,7 @@ ${token.value}`,
}
});

test('There should be no obsolete ESLint output snapshots', () => {
it('There should be no obsolete ESLint output snapshots', () => {
const files = fs.readdirSync(eslintOutputSnapshotFolder);
const names = new Set(Object.keys(rules).map(k => `${k}.shot`));

Expand Down
4 changes: 2 additions & 2 deletions packages/eslint-plugin/tests/rules/array-type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2232,14 +2232,14 @@ type BrokenArray = {

describe('schema validation', () => {
// https://github.com/typescript-eslint/typescript-eslint/issues/6852
test("array-type does not accept 'simple-array' option", () => {
it("array-type does not accept 'simple-array' option", () => {
if (areOptionsValid(rule, [{ default: 'simple-array' }])) {
throw new Error(`Options succeeded validation for bad options`);
}
});

// https://github.com/typescript-eslint/typescript-eslint/issues/6892
test('array-type does not accept non object option', () => {
it('array-type does not accept non object option', () => {
if (areOptionsValid(rule, ['array'])) {
throw new Error(`Options succeeded validation for bad options`);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/tests/rules/no-unnecessary-template-expression.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -964,7 +964,7 @@ describe('fixer should not change runtime value', () => {
continue;
}

test(code, () => {
it(code, () => {
expect(eval(code)).toEqual(
eval(Array.isArray(output) ? output.at(-1)! : output),
);
Expand Down
6 changes: 3 additions & 3 deletions packages/eslint-plugin/tests/schemas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ describe('Rule schemas should be convertible to TS types for documentation purpo
}
});

test('There should be no old snapshots for rules that have been deleted', () => {
it('There should be no old snapshots for rules that have been deleted', () => {
const files = fs.readdirSync(snapshotFolder);
const names = new Set(
Object.keys(rules)
Expand Down Expand Up @@ -191,7 +191,7 @@ describe('Rule schemas should validate options correctly', () => {
};

for (const [ruleName, rule] of Object.entries(rules)) {
test(`${ruleName} must accept valid options`, () => {
it(`${ruleName} must accept valid options`, () => {
if (
!areOptionsValid(
rule,
Expand All @@ -202,7 +202,7 @@ describe('Rule schemas should validate options correctly', () => {
}
});

test(`${ruleName} rejects arbitrary options`, () => {
it(`${ruleName} rejects arbitrary options`, () => {
if (areOptionsValid(rule, [{ 'arbitrary-schemas.test.ts': true }])) {
throw new Error(`Options succeeded validation for arbitrary options`);
}
Expand Down
Loading