Add no-ignored-test-files rule (fixes #51)

[jfmengels]

Add no-ignored-test-files rule (fixes #51)

I would like to add more checks to see whether the test is in the AVA glob (with an option to de-/activate or set a custom glob), but I haven't found (yet, haven't searched much for now) a library that simply tells me whether a file matches a glob, and I don't know if we can reliably get the cwd from ESLint (or the editor running ESLint).
This can be added later to the rule though. I might take a look at it later, or someone could take a look too if interested.

[novemberborn]

I would like to add more checks to see whether the test is in the AVA glob (with an option to de-/activate or set a custom glob), but I haven't found (yet, haven't searched much for now) a library that simply tells me whether a file matches a glob

The watcher does this, see https://github.com/sindresorhus/ava/blob/005b656b9950ab3fa1e2bd9af326d5b39540e71b/lib/watcher.js#L333:L379. It uses https://www.npmjs.com/package/multimatch.

[jfmengels]

From what I could test, in Atom's ESLint plugin (the Atom plugin that runs ESLint I mean), process.cwd() returns the current file's folder everytime, not the root of the project, and ESLint's context.getFilename() returns the file's absolute path. Meaning it becomes pretty hard to figure out the project's root folder.

So it means two things:

• Globbing becomes pretty hard, because even the default glob is relative to the folder Ava is run from.
• Anybody who uses eslint-plugin-ava (at least in Atom) will have this rule ringing loudly for every file if his project, dev folder, or any higher-up folder is named fixtures/helpers.

The second is obviously pretty rare (I'd think at least), but it can be problematic for some users. Let me know what you guys think about this.

[novemberborn]

@jfmengels could use https://www.npmjs.com/package/pkg-up until you find a package that depends on AVA?

[jfmengels]

@novemberborn Sounded good, so I tried it and it works well. I disabled the rule altogether progammatically when no package.json is found (debatable, but probably pretty rare anyway. The rule is not worth much without it, nor all that reliable).

The result is pretty rad IMO!

Haven't squashed the commits so that I could revert back to previous impl, as someone might find it unacceptable to depend on a package.json file. I'll squash and merge when people are happy with this.

[novemberborn]

Why not use multimatch here as well? Copy excludePatterns without the leading ! and you should be good to go.

[jfmengels]

Sounds good! Will do that (won't put node_modules though as ESLint ignores them already)

[novemberborn]

Do people put test factories in their helpers? That might be a legitimate ava import. Though you could disable the rule in that case or customize the pattern.

---

Very cool @jfmengels!

[jfmengels]

@novemberborn @jamestalmage had the same comment #51 (comment), and I agree that you should then simply disable the rule. There was an interesting proposition avajs/ava#695 for test macros which would be better. We only lint an error if you import AVA and create a test, but we could then make an exception if you're defining a macro

[jfmengels]

Updated :)

[novemberborn]

We only lint an error if you import AVA and create a test, but we could then make an exception if you're defining a macro

That sounds good.

[novemberborn]

Mention node_modules here?

[jfmengels]

Well, ESLint ignores node_modules anyway. I don't really mind adding it, but for consistency we'd have to add it to every rule. I think that users of Node or ESLint know that node_modules/ should not be touched.

[novemberborn]

Yes but you're describing AVA's behavior here.

[jfmengels]

True. I've added the mention about node_modules, and to clarify, a message saying that they won't be linted due to ESLint.

[novemberborn]

Nothing further from me 👯

[sindresorhus]

LGTM

[jfmengels]

Cool, I'll squash and merge then.

[jamestalmage]

@jfmengels - FYI - GH has a new "Squash and Merge" feature built in. Just click the merge button and you will see a drop down that gives you that option.

Ideally you would use that, even for 1 commit PR's. It avoids creating an extra merge commit.

If there are multiple commits that you want to keep separate, the old merge method is fine.

[jfmengels]

Aah. Yeah I knew about it, but I thought it was something that you needed to activate as I didn't see it. But it only shows when you click the merge button once apparently ^^'

(Hoping for a rebase button to come soon)

[jamestalmage]

You usually only need to rebase if there is a merge conflict (in which case you probably want to do some edits / testing locally). I guess it could be useful for selectively squashing (i.e. ending up with less commits, but still more than one), and for editing commit messages. But that is a rare enough need I'm not clamoring for it. I would much rather see improvements to issue management first.

[jfmengels]

I mean merge rebase in the sense of no merge commits. I like straight lines.

[sindresorhus]

I mean merge rebase in the sense of no merge commits. I like straight lines.

That's exactly what the squash button does, but you seems you used the old merge button.