Skip to content

feat(rules): option for rule to allow uppercase for top level describes #428

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ node_modules/
coverage/
lib/
*.log
package-lock.json
36 changes: 35 additions & 1 deletion src/rules/__tests__/lowercase-name.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { TSESLint } from '@typescript-eslint/experimental-utils';
import rule from '../lowercase-name';
import { DescribeAlias, TestCaseName } from '../utils';
import { DescribeAlias, TestCaseName, TopCase } from '../utils';

const ruleTester = new TSESLint.RuleTester({
parserOptions: {
Expand Down Expand Up @@ -225,3 +225,37 @@ ruleTester.run('lowercase-name with ignore=it', rule, {
],
invalid: [],
});

ruleTester.run('uppercase-name with ignore=top', rule, {
valid: [
{
code: `describe("Foo", () => {
test("bar", () => {
some_fn();
})
})`,
options: [{ ignore: [TopCase.top] }],
},
{
code: `describe("Foo", () => {
it("bar", () => {
some_fn();
})
})`,
options: [{ ignore: [TopCase.top] }],
},
{
code: 'describe(`Foo`, function () {})',
options: [{ ignore: [TopCase.top] }],
},
{
code: 'test(`Foo`, function () {})',
options: [{ ignore: [TopCase.top] }],
},
{
code: 'it(`Foo`, function () {})',
options: [{ ignore: [TopCase.top] }],
},
],
invalid: [],
});
44 changes: 41 additions & 3 deletions src/rules/lowercase-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
} from '@typescript-eslint/experimental-utils';
import {
DescribeAlias,
TopCase,
JestFunctionCallExpressionWithIdentifierCallee,
JestFunctionName,
TestCaseName,
Expand Down Expand Up @@ -52,6 +53,32 @@ const testDescription = (argument: ArgumentLiteral): string | null => {
return argument.quasis[0].value.raw;
};

const jestTopFunctioName = (
node: CallExpressionWithCorrectCalleeAndArguments,
counter: number,
) => {
const description = testDescription(node.arguments[0]);
if (description === null) {
return null;
}

if (counter !== 1) {
return null;
}

const firstCharacter = description.charAt(0);

if (!firstCharacter) {
return null;
}

if (firstCharacter === firstCharacter.toUpperCase()) {
return node.callee.name;
}

return null;
}

const jestFunctionName = (
node: CallExpressionWithCorrectCalleeAndArguments,
) => {
Expand Down Expand Up @@ -93,7 +120,7 @@ export default createRule({
properties: {
ignore: {
type: 'array',
items: { enum: ['describe', 'test', 'it'] },
items: { enum: ['describe', 'test', 'it', 'top'] },
Copy link
Collaborator

@G-Rath G-Rath Oct 5, 2019

Choose a reason for hiding this comment

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

I think if you're going to do it this way, it would be better to use a boolean option since top isn't a jest keyword and so doesn't really make sense having it implemented in the same manner of isTestCase & isDescribe, nor in the JestFunctionName union.

That would also be a good position to later extend it to be an array accepting the same properties as ignore.

Copy link
Author

@gayathrigs27 gayathrigs27 Oct 8, 2019

Choose a reason for hiding this comment

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

My understanding is that if I gave ignore=top, the lowercase rule will not apply to the first parent element(describe/test/it) the inner describe/test should have this rule.
If I do an early return as you suggested the inner elements will also be skipped off the rule. Which is

describe("Xyz abc"){
  test("Cde"){}
}

will be a valid one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear, this is the sort of early return I had in mind:

if(ignoreTopLevel && isTopMostJestFunction(node)) {
  return;
}

Since CallExpression is called for every CallExpression that ESLint finds, the early return won't interfere with the inner jest function calls.

additionalItems: false,
},
},
Expand All @@ -103,14 +130,25 @@ export default createRule({
} as const,
defaultOptions: [{ ignore: [] } as { ignore: readonly JestFunctionName[] }],
create(context, [{ ignore }]) {
let counter: number = 0
return {

CallExpression(node) {
if (!isJestFunctionWithLiteralArg(node)) {
return;
}

if (isTestCase || isDescribe) {
counter += 1
}

const topMethod = jestTopFunctioName(node, counter);

const topMethodIgnore = topMethod && ignore.includes(TopCase.top);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only value being used in this conditional that is computed as part of the CallExpression function is topMethod, meaning you could move ignore.includes(TopCase.top) out of this function, and assign it to a variable like you do w/ counter.

That way we won't be iterating over ignores for every CallExpression that makes it this far - a micro optimisation for sure, but one that's zero-effort, so might as well.


const erroneousMethod = jestFunctionName(node);

if (erroneousMethod && !ignore.includes(node.callee.name)) {
if (erroneousMethod && !ignore.includes(node.callee.name) && topMethodIgnore!==true) {
context.report({
messageId: 'unexpectedLowercase',
data: { method: erroneousMethod },
Expand All @@ -137,4 +175,4 @@ export default createRule({
},
};
},
});
});
6 changes: 5 additions & 1 deletion src/rules/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,11 @@ export enum TestCaseProperty {
'todo' = 'todo',
}

export type JestFunctionName = DescribeAlias | TestCaseName | HookName;
export enum TopCase {
'top' = 'top',
}

export type JestFunctionName = DescribeAlias | TestCaseName | HookName | TopCase;

export interface JestFunctionIdentifier<FunctionName extends JestFunctionName>
extends TSESTree.Identifier {
Expand Down