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 all commits
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"typecheck": "tsc -p ."
},
"dependencies": {
"@typescript-eslint/experimental-utils": "^1.13.0"
"@typescript-eslint/experimental-utils": "^1.13.0",
"build": "^0.1.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need build?

},
"devDependencies": {
"@babel/cli": "^7.4.4",
Expand Down
47 changes: 46 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 @@ -230,6 +230,51 @@ ruleTester.run('lowercase-name with ignore=it', rule, {
invalid: [],
});

ruleTester.run('uppercase-name with ignore=top', rule, {
valid: [
{
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] }],
},
{
code: `describe('MyFirstClass', () => {});
describe('MySecondClass', () => {});`,
options: [{ ignore: [TopCase.top] }],
},
],
invalid: [
{
code: `describe("Foo", () => {test("Bar foo", () => {})})`,
options: [{ ignore: [TopCase.top] }],
output: `describe("Foo", () => {test("bar foo", () => {})})`,
errors: [
{
messageId: 'unexpectedLowercase',
data: { method: TestCaseName.test },
column: 24,
line: 1,
},
],
},
],
});

ruleTester.run('lowercase-name with allowedPrefixes', rule, {
valid: [
{
Expand Down
32 changes: 31 additions & 1 deletion src/rules/lowercase-name.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import {
JestFunctionCallExpressionWithIdentifierCallee,
JestFunctionName,
TestCaseName,
TopCase,
createRule,
isDescribe,
isTestCase,
isTopMostJestFunction,
} from './utils';

type ArgumentLiteral = TSESTree.Literal | TSESTree.TemplateLiteral;
Expand Down Expand Up @@ -52,6 +54,27 @@ const testDescription = (argument: ArgumentLiteral): string | null => {
return argument.quasis[0].value.raw;
};

const jestTopFunctioName = (
node: CallExpressionWithCorrectCalleeAndArguments,
isTop: boolean,
) => {
const description = testDescription(node.arguments[0]);
if (isTop !== true) {
return null;
}

if (description === null) {
return null;
}
const firstCharacter = description.charAt(0);

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

return null;
};

const jestFunctionName = (
node: CallExpressionWithCorrectCalleeAndArguments,
allowedPrefixes: readonly string[],
Expand Down Expand Up @@ -105,7 +128,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,
},
allowedPrefixes: {
Expand All @@ -118,13 +141,20 @@ export default createRule<
},
],
} as const,

defaultOptions: [{ ignore: [], allowedPrefixes: [] }],
create(context, [{ ignore = [], allowedPrefixes = [] }]) {
return {
CallExpression(node) {
if (!isJestFunctionWithLiteralArg(node)) {
return;
}

const topMethod = jestTopFunctioName(node, isTopMostJestFunction(node));
if (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.

Could you replace TopCase.top w/ an ignoreTopLevel option, as previously discussed?

You can also inline topMethod, since it's not used elsewhere :)

return;
}

const erroneousMethod = jestFunctionName(node, allowedPrefixes);

if (erroneousMethod && !ignore.includes(node.callee.name)) {
Expand Down
17 changes: 16 additions & 1 deletion src/rules/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,15 @@ 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 Expand Up @@ -652,6 +660,13 @@ export const isDescribe = (
);
};

export function isTopMostJestFunction(node: TSESTree.CallExpression): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer that this be inside the rule, rather than in utils - iirc there is another rule doing something similar, but I'm going to be doing a spring-cleaning of the codebase this weekend, so I can refactor this into a utility method in that PR.

For now, it'll just be cleaner to have it in the rule :)

if (node.parent && node.parent.parent && node.parent.parent.parent === null) {
return true;
}
return false;
}

const collectReferences = (scope: TSESLint.Scope.Scope) => {
const locals = new Set();
const unresolved = new Set();
Expand Down
Loading