Add `no-import-test-file` rule #193

HenriBeck · 2018-06-01T12:55:20Z

Fixes #141

Created a rule that forbids test files in any form.

novemberborn

Thanks for the PR @HenriBeck! I suppose this needs #192 for ava.config.js support?

Could you add documentation for the new rule?

@avajs/core should this be a recommended rule?

(Also, I'm not really familiar with how ESLint plugins work, so if anybody else could have a look at this PR that'd be great.)

novemberborn · 2018-06-11T17:06:28Z

rules/no-import-test-files.js

+	const absoluteImportedPath = path.resolve(path.dirname(sourceFile), importedFile);
+	const relativePath = path.relative(rootDir, absoluteImportedPath);
+
+	console.log(sourceFile, importedFile, absoluteImportedPath, relativePath);


Could you remove this line?

Ohh, yeah totally forgot that line.

novemberborn · 2018-06-11T17:07:42Z

rules/no-import-test-files.js

+			if (isImportingTestFile) {
+				context.report({
+					node,
+					message: `You are importing a test file`,


This doesn't seem like the most helpful message, but I'm not really familiar with how this shows up in linter reports. @sindresorhus?

If you have a better commit message, feel free to suggest it.

It shows up as the message in the linter at the specific line.

novemberborn · 2018-06-11T17:09:20Z

rules/no-import-test-files.js

+		return {};
+	}
+
+	const packageInfo = getPackageInfo();


Could you rename this to projectInfo / getProjectInfo() etc? That better aligns with how things are named in AVA itself.

- Renamed function getPackageInfo to getProjectInfo Signed-off-by: Henri Beck <[email protected]>

Signed-off-by: Henri Beck <[email protected]>

HenriBeck · 2018-06-16T13:06:40Z

No, this PR doesn't necessarily need #192.

Personally, I would put that into the recommended config, because I think this is a pretty clear anti-pattern.

sindresorhus · 2018-06-24T08:20:52Z

@avajs/core should this be a recommended rule?

Yes, this should be a recommended rule.

sindresorhus · 2018-06-24T08:24:34Z

This rule needs to be added to the readme.

sindresorhus · 2018-06-24T08:22:33Z

rules/no-import-test-files.js

+	'test/**/*.js',
+	'**/__tests__/**/*.js',
+	'**/*.test.js'
+];


This should be moved to util as it's also defined in no-ignored-test-files (the one there should be moved too)

sindresorhus · 2018-06-24T08:26:18Z

docs/rules/no-import-test-files.md

@@ -0,0 +1,60 @@
+# Ensure no tests are imported anywhere


tests => test files

sindresorhus · 2018-06-24T08:27:22Z

docs/rules/no-import-test-files.md

+# Ensure no tests are imported anywhere
+
+This rule will verify that you don't import any test files. It will consider the root of the project to be the closest folder containing a `package.json` file, and will not do anything if it can't find one. Test files in `node_modules` will not be linted as they are ignored by ESLint.
+


The rule should mention why it's bad to import test files, like described in #141

sindresorhus · 2018-06-24T08:28:06Z

docs/rules/no-import-test-files.md

+
+This rule will verify that you don't import any test files. It will consider the root of the project to be the closest folder containing a `package.json` file, and will not do anything if it can't find one. Test files in `node_modules` will not be linted as they are ignored by ESLint.
+
+Note that this rule will not be able to warn correctly if you use AVA by specifying the files in the command line ( `ava "lib/**/*.test.js"` ). Prefer configuring AVA as described in the link above.


sindresorhus · 2018-06-24T08:29:12Z

docs/rules/no-import-test-files.md

+// Invalid because *.test.js is considered as a test file.
+import tests from './index.test.js';
+
+// File: src/index.js


It's confusing that you have multiple file examples in the same code block. They should be separate code blocks.

sindresorhus · 2018-06-24T08:30:48Z

rules/no-import-test-files.js

+			if (isImportingTestFile) {
+				context.report({
+					node,
+					message: `You are importing a test file`,


Test files should not be imported

sindresorhus · 2018-06-24T08:31:31Z

rules/no-import-test-files.js

+			}
+		},
+		CallExpression: node => {
+			if (node.callee.type !== 'Identifier' || node.callee.name !== 'require') {


It's clearer to write it like this:

if (!(node.callee.type === 'Identifier' && node.callee.name === 'require')) {

Same with the below.

sindresorhus · 2018-06-24T08:33:13Z

rules/no-import-test-files.js

+					node,
+					message: `You are importing a test file`,
+				});
+			}


Line 50 to 57 is almost duplicated with the CallExpression one. Just make an abstracted method that accepts the value.

sindresorhus · 2018-06-24T08:35:44Z

rules/no-import-test-files.js

+		files: {
+			anyOf: [
+				{type: 'array'},
+				{type: 'string'}


string should not be allowed

Signed-off-by: Henri Beck <[email protected]>

sindresorhus · 2018-06-27T05:39:39Z

readme.md

@@ -74,6 +75,7 @@ The rules will only activate in test files.
 - [no-duplicate-modifiers](docs/rules/no-duplicate-modifiers.md) - Ensure tests do not have duplicate modifiers.
 - [no-identical-title](docs/rules/no-identical-title.md) - Ensure no tests have the same title.
 - [no-ignored-test-files](docs/rules/no-ignored-test-files.md) - Ensure no tests are written in ignored files.
+- [no-import-test-files](docs/rules/no-import-test-files.md) - Ensure no tests files are imported anywhere.


tests => test

sindresorhus · 2018-06-27T06:29:44Z

index.js

@@ -37,7 +37,8 @@ module.exports = {
 				'ava/use-t-well': 'error',
 				'ava/use-t': 'error',
 				'ava/use-test': 'error',
-				'ava/use-true-false': 'error'
+				'ava/use-true-false': 'error',
+				'ava/no-import-test-files': 'error',


This needs to be in the same position as placed in the readme.

Signed-off-by: Henri Beck <[email protected]>

sindresorhus · 2018-07-14T18:30:12Z

Nice work again, @HenriBeck 🙌

novemberborn · 2018-07-15T13:32:39Z

Thanks @HenriBeck! If you're up for another challenge, we should support custom extensions with this rule. See #201.

HenriBeck · 2018-07-15T14:13:13Z

Will check it out next week.

Added no-import-test-file

8f21dd4

novemberborn requested changes Jun 11, 2018

View reviewed changes

Henri Beck added 2 commits June 16, 2018 11:01

- Removed console.log

ca72a65

- Renamed function getPackageInfo to getProjectInfo Signed-off-by: Henri Beck <[email protected]>

Added documentation

1365ba7

Signed-off-by: Henri Beck <[email protected]>

sindresorhus changed the title ~~Added no-import-test-file~~ Add no-import-test-file rule Jun 24, 2018

Update no-import-test-files.js

5d23800

sindresorhus requested changes Jun 24, 2018

View reviewed changes

Henri Beck added 7 commits June 26, 2018 10:08

Added import-no-test-files to README

2e15713

Signed-off-by: Henri Beck <[email protected]>

Moved defaultFiles to utils file

6814832

Signed-off-by: Henri Beck <[email protected]>

Updated option schema

4cdb548

Signed-off-by: Henri Beck <[email protected]>

Improved rule implementation

06c223f

Signed-off-by: Henri Beck <[email protected]>

Fixed some lint errors

6616129

Signed-off-by: Henri Beck <[email protected]>

Updated rules documentation

c2fc88c

Signed-off-by: Henri Beck <[email protected]>

Fixed tests

3d46dcf

Signed-off-by: Henri Beck <[email protected]>

sindresorhus requested changes Jun 27, 2018

View reviewed changes

Henri Beck and others added 4 commits June 27, 2018 09:00

Fixed typo

2bfa9aa

Signed-off-by: Henri Beck <[email protected]>

Fixed ordering of recommended rules

42406d0

Signed-off-by: Henri Beck <[email protected]>

Fixed linting

fe8958b

Signed-off-by: Henri Beck <[email protected]>

Update index.js

d1acaf5

sindresorhus force-pushed the master branch 2 times, most recently from befdb58 to 6a6706e Compare July 14, 2018 18:27

sindresorhus merged commit 0134e94 into avajs:master Jul 14, 2018

HenriBeck deleted the add-no-import-test-files-rule branch July 14, 2018 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `no-import-test-file` rule #193

Add `no-import-test-file` rule #193

HenriBeck commented Jun 1, 2018 •

edited by sindresorhus

Loading

novemberborn left a comment

novemberborn Jun 11, 2018

HenriBeck Jun 11, 2018

novemberborn Jun 11, 2018

HenriBeck Jun 16, 2018

novemberborn Jun 11, 2018

HenriBeck commented Jun 16, 2018

sindresorhus commented Jun 24, 2018

sindresorhus commented Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 24, 2018

sindresorhus Jun 27, 2018

sindresorhus Jun 27, 2018

sindresorhus commented Jul 14, 2018

novemberborn commented Jul 15, 2018

HenriBeck commented Jul 15, 2018

		# Ensure no tests are imported anywhere

		This rule will verify that you don't import any test files. It will consider the root of the project to be the closest folder containing a `package.json` file, and will not do anything if it can't find one. Test files in `node_modules` will not be linted as they are ignored by ESLint.


		This rule will verify that you don't import any test files. It will consider the root of the project to be the closest folder containing a `package.json` file, and will not do anything if it can't find one. Test files in `node_modules` will not be linted as they are ignored by ESLint.

		Note that this rule will not be able to warn correctly if you use AVA by specifying the files in the command line ( `ava "lib/*/.test.js"` ). Prefer configuring AVA as described in the link above.

Add no-import-test-file rule #193

Add no-import-test-file rule #193

Conversation

HenriBeck commented Jun 1, 2018 • edited by sindresorhus Loading

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HenriBeck commented Jun 16, 2018

sindresorhus commented Jun 24, 2018

sindresorhus commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jul 14, 2018

novemberborn commented Jul 15, 2018

HenriBeck commented Jul 15, 2018

Add `no-import-test-file` rule #193

Add `no-import-test-file` rule #193

HenriBeck commented Jun 1, 2018 •

edited by sindresorhus

Loading