Skip to content
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

feat(no-export): new rule for no-export #307

Merged
merged 6 commits into from Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -114,6 +114,7 @@ installations requiring long-term consistency.
| [no-commented-out-tests][] | Disallow commented out tests | | |
| [no-duplicate-hooks][] | Disallow duplicate hooks within a `describe` block | | |
| [no-empty-title][] | Disallow empty titles | | |
| [no-export][] | Disallow export from test files | ![recommended][] | |
Copy link
Member

Choose a reason for hiding this comment

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

Not recommended (changing recommended rules is a breaking change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's the process of getting it to be a recommended change? This rule prevents very hard to track down problems....

Copy link
Member

Choose a reason for hiding this comment

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

Just remembering when we release a new major :)

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'll remove it for now, but I would like to move this to recommended in another PR, if you don't mind providing guidance on that process.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Just open up a PR updating the readme and add it to the config exported from index and I'll make sure to merge it before making a major release

| [no-focused-tests][] | Disallow focused tests | ![recommended][] | |
| [no-hooks][] | Disallow setup and teardown hooks | | |
| [no-identical-title][] | Disallow identical titles | ![recommended][] | |
Expand Down Expand Up @@ -163,6 +164,7 @@ https://github.com/dangreenisrael/eslint-plugin-jest-formatting
[no-duplicate-hooks]: docs/rules/no-duplicate-hooks.md
[no-commented-out-tests]: docs/rules/no-commented-out-tests.md
[no-empty-title]: docs/rules/no-empty-title.md
[no-export]: docs/rules/no-export.md
[no-focused-tests]: docs/rules/no-focused-tests.md
[no-hooks]: docs/rules/no-hooks.md
[no-identical-title]: docs/rules/no-identical-title.md
Expand Down
46 changes: 46 additions & 0 deletions docs/rules/no-export.md
@@ -0,0 +1,46 @@
# no export from test file (no-export)

Prevents exports from test files. If a file has at least 1 test in it, then this
rule will prevent exports.

## Rule Details

This rule aims to eliminate duplicate runs of tests by exporting things from
test files. If you import from a test file, then all the tests in that file will
be run in each imported instance, so bottom line, don't export from a test, but
instead move helper functions into a seperate file when they need to be shared
across tests.

Examples of **incorrect** code for this rule:

```js
export function myHelper() {}

module.exports = function() {};

module.exports = {
something: 'that should be moved to a non-test file',
};

describe('a test', () => {
expect(1).toBe(1);
});
```

Examples of **correct** code for this rule:

```js
function myHelper() {}

const myThing = {
something: 'that can live here',
};

describe('a test', () => {
expect(1).toBe(1);
});
```

## When Not To Use It

Don't use this rule on non-jest test files.
2 changes: 1 addition & 1 deletion src/__tests__/rules.test.js
Expand Up @@ -3,7 +3,7 @@ import { resolve } from 'path';
import { rules } from '../';

const ruleNames = Object.keys(rules);
const numberOfRules = 34;
const numberOfRules = 35;

describe('rules', () => {
it('should have a corresponding doc for each rule', () => {
Expand Down
50 changes: 50 additions & 0 deletions src/rules/__tests__/no-export.test.js
@@ -0,0 +1,50 @@
import { RuleTester } from 'eslint';
import rule from '../no-export';

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 2015,
sourceType: 'module',
},
});

ruleTester.run('no-export', rule, {
valid: [
'describe("a test", () => { expect(1).toBe(1); })',
'window.location = "valid"',
'module.somethingElse = "foo";',
'export const myThing = "valid"',
'export default function () {}',
'module.exports = function(){}',
'module.exports.myThing = "valid";',
],
invalid: [
{
code:
'export const myThing = "invalid"; test("a test", () => { expect(1).toBe(1);});',
parserOptions: { sourceType: 'module' },
errors: [{ endColumn: 34, column: 1, messageId: 'unexpectedExport' }],
},
{
code:
'export default function() {}; test("a test", () => { expect(1).toBe(1);});',
parserOptions: { sourceType: 'module' },
errors: [{ endColumn: 29, column: 1, messageId: 'unexpectedExport' }],
},
{
code:
'module.exports["invalid"] = function() {}; test("a test", () => { expect(1).toBe(1);});',
errors: [{ endColumn: 26, column: 1, messageId: 'unexpectedExport' }],
},
{
code:
'module.exports = function() {}; ; test("a test", () => { expect(1).toBe(1);});',
errors: [{ endColumn: 15, column: 1, messageId: 'unexpectedExport' }],
},
{
code:
'module.export.invalid = function() {}; ; test("a test", () => { expect(1).toBe(1);});',
errors: [{ endColumn: 22, column: 1, messageId: 'unexpectedExport' }],
},
],
});
49 changes: 49 additions & 0 deletions src/rules/no-export.js
@@ -0,0 +1,49 @@
import { getDocsUrl, isTestCase } from './util';

let exportNodes = [];
benmonro marked this conversation as resolved.
Show resolved Hide resolved
let hasTestCase = false;
const messageId = 'unexpectedExport';
export default {
meta: {
docs: {
url: getDocsUrl(__filename),
},
messages: {
[messageId]: `Do not export from a test file.`,
benmonro marked this conversation as resolved.
Show resolved Hide resolved
},
schema: [],
},
create(context) {
return {
'Program:exit'() {
if (hasTestCase && exportNodes.length > 0) {
for (let node of exportNodes) {
context.report({ node, messageId });
}
}
exportNodes = [];
hasTestCase = false;
},

CallExpression(node) {
if (isTestCase(node)) {
hasTestCase = true;
}
},
'ExportNamedDeclaration, ExportDefaultDeclaration'(node) {
exportNodes.push(node);
},
'AssignmentExpression > MemberExpression'(node) {
let { object, property } = node;

if (object.type === 'MemberExpression') {
({ object, property } = object);
}

if (object.name === 'module' && !!property.name.match(/^exports?$/)) {
benmonro marked this conversation as resolved.
Show resolved Hide resolved
exportNodes.push(node);
}
},
};
},
};