Skip to content

ts-migration/consistent-test-it #327

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

Merged
merged 3 commits into from
Jul 20, 2019
Merged

ts-migration/consistent-test-it #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99e8c30
chore(consistent-test-it): convert to typescript
G-Rath Jul 20, 2019
35785d7
chore(consistent-test-it): use `AST_NODE_TYPES` enum
G-Rath Jul 20, 2019
60479fb
chore(consistent-test-it): use union instead of string
G-Rath Jul 20, 2019
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
4 changes: 2 additions & 2 deletions ...ules/__tests__/consistent-test-it.test.js → ...ules/__tests__/consistent-test-it.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { RuleTester } from 'eslint';
import { TSESLint } from '@typescript-eslint/experimental-utils';
import rule from '../consistent-test-it';

const ruleTester = new RuleTester({
const ruleTester = new TSESLint.RuleTester({
parserOptions: {
ecmaVersion: 6,
},
Expand Down
31 changes: 23 additions & 8 deletions src/rules/consistent-test-it.js → src/rules/consistent-test-it.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { getDocsUrl, getNodeName, isDescribe, isTestCase } from './util';
import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils';
import { createRule, getNodeName, isDescribe, isTestCase } from './tsUtils';

export default {
export default createRule({
name: __filename,
meta: {
docs: {
url: getDocsUrl(__filename),
category: 'Best Practices',
description: 'Have control over `test` and `it` usages',
recommended: false,
},
fixable: 'code',
messages: {
Expand All @@ -26,7 +30,14 @@ export default {
additionalProperties: false,
},
],
type: 'suggestion',
},
defaultOptions: [
{ fn: 'test', withinDescribe: 'it' } as {
fn?: 'it' | 'test';
Copy link
Member

@SimenB SimenB Jul 20, 2019

Choose a reason for hiding this comment

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

do we need as const on the schema?

Copy link
Member

@SimenB SimenB Jul 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

might not need the type info here then. Maybe not though, not sure how enum in schema works

Copy link
Collaborator Author

@G-Rath G-Rath Jul 20, 2019

Choose a reason for hiding this comment

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

Not as far as I'm aware - the schema is json schema, so it's not actually used by TS (it'd be cool if it was, but it's far too complex)

withinDescribe?: 'it' | 'test';
},
],
create(context) {
const configObj = context.options[0] || {};
const testKeyword = configObj.fn || 'test';
Expand All @@ -39,6 +50,10 @@ export default {
CallExpression(node) {
const nodeName = getNodeName(node.callee);

if (!nodeName) {
return;
Copy link
Member

@SimenB SimenB Jul 20, 2019

Choose a reason for hiding this comment

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

new, but seeing as tests pass I guess it doesn't matter.

Copy link
Collaborator Author

@G-Rath G-Rath Jul 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's required by typescript - getNodeName could always return null, but not always checked:

export const getNodeName = node => {
  function joinNames(a, b) {
    return a && b ? `${a}.${b}` : null;
  }

  switch (node && node.type) {
    case 'Identifier':
      return node.name;
    case 'MemberExpression':
      return joinNames(getNodeName(node.object), getNodeName(node.property));
  }

  return null;
};

Ah, the joys of complex ASTs 😂

Copy link
Collaborator Author

@G-Rath G-Rath Jul 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

seeing as tests pass I guess it doesn't matter.

I believe this is b/c of the isDescribe - the conditions for which getNodeName would return null are mutually exclusive w/ isDescribe, which is why this has never been a problem.

I mean, that branch must be being hit b/c coverage isn't complaining, so it can happen.

Copy link
Collaborator Author

@G-Rath G-Rath Jul 20, 2019

Choose a reason for hiding this comment

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

Just so it's written down somewhere, this is the code that can be used to make getNodeName to return null:

[1,2,3].forEach(() => { test("foo") })

It's b/c [1,2,3] is an ArrayExpression, which causes getNodeName to bail out.

I needed to know how to do this to implement a test for another rule to get coverage other the line :)

}

if (isDescribe(node)) {
describeNestingLevel++;
}
Expand All @@ -56,7 +71,7 @@ export default {
data: { testKeyword, oppositeTestKeyword },
fix(fixer) {
const nodeToReplace =
node.callee.type === 'MemberExpression'
node.callee.type === AST_NODE_TYPES.MemberExpression
? node.callee.object
: node.callee;

Expand All @@ -81,7 +96,7 @@ export default {
data: { testKeywordWithinDescribe, oppositeTestKeyword },
fix(fixer) {
const nodeToReplace =
node.callee.type === 'MemberExpression'
node.callee.type === AST_NODE_TYPES.MemberExpression
? node.callee.object
: node.callee;

Expand All @@ -101,9 +116,9 @@ export default {
},
};
},
};
});

function getPreferredNodeName(nodeName, preferredTestKeyword) {
function getPreferredNodeName(nodeName: string, preferredTestKeyword: string) {
switch (nodeName) {
case 'fit':
return 'test.only';
Expand All @@ -114,7 +129,7 @@ function getPreferredNodeName(nodeName, preferredTestKeyword) {
}
}

function getOppositeTestKeyword(test) {
function getOppositeTestKeyword(test: string) {
if (test === 'test') {
return 'it';
}
Expand Down