-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
Update no-test-import-export
rule to allow importing from anything under tests/helpers
path
#895
Update no-test-import-export
rule to allow importing from anything under tests/helpers
path
#895
Conversation
…under tests/helpers path
importSource.startsWith('tests/helpers/') || | ||
importSource.includes('/tests/helpers/') || | ||
importSource.endsWith('/tests/helpers') || | ||
importSource === 'tests/helpers' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid for tests/helpers
to be a single file instead of a directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I suppose this line would handle either a file named tests/helpers.js
or a file named tests/helpers/index.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid for tests/helpers to be a single file instead of a directory?
Yes, it is definitely possible (and allowed) to use a single tests/helpers.js
file or a tests/helpers/index.js
file.
@@ -42,7 +45,7 @@ module.exports = { | |||
ImportDeclaration(node) { | |||
const importSource = node.source.value; | |||
|
|||
if (importSource.endsWith('-test')) { | |||
if (importSource.endsWith('-test') && !isTestHelperImportSource(importSource)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not 100% sure this is correct. We shouldn't be importing from tests/helpers/something-test.js
(e.g. I think the first condition was fine, any file ending with -test
is itself a test file not a helper file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why it was done. If you literally write a module like tests/helpers/some-verb-test.js
it would still error.
}, | ||
|
||
// Importing anything from tests/helpers is allowed. | ||
"import setupApplicationTest from 'tests/helpers/setup-application-test.js';", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing relative path test case, for example:
../../helpers/setup-application-test.js
--
I upgraded eslint-plugin-ember to v8.10.1 and my ESLint still complains about imports from helpers folder, because relative import paths are being used.
I can workaround by changing to absolute path (TypeScript app), like myapp/tests/helpers/
.
It seems plugin should resolve real path on disk with path.resolve
or something to determine truthiness in isTestHelperFilename
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix it: #911
Fixes #889.
CC: @mongoose700 @raido