Protect us from forgetting to remove .skip() or .only()

[sholladay]

Description

This is an idea for a feature to help alleviate problems with forgetting to remove .skip() and .only() in tests.

Problem

It is a common practice to run tests during development, but even the most diligent of us (and all of our fancy automation) can be easily fooled into thinking everything is okay when it isn't, by simple human error, when you forget to remove .only() / .skip() from a test. It is easy to end up in a situation where you ship broken code because the tests technically succeeded but lots of tests were not actually run and the broken code didn't even get exercised at all.

Currently, AVA prints a reminder to the console when you use these methods, which is useful if you happen to notice it. But in some cases, that may not even be possible. For example, a common way to run npm test is via git hooks, but my favorite GUI for git, Tower, only shows output from hooks when they fail. I like it that way, in general, though that means AVA's logs cannot be seen and thus it is even less obvious that some tests were not run.

Solution

One of the better solutions I have seen so far to prevent leaving in skip modifies is to grep for them inside of a pre-push hook, as documented by @jegtnes here:

https://jegtnes.com/blog/use-git-pre-push-hooks-to-stop-test-suite-skips/

That got me thinking... AVA could make this nearly foolproof because it actually knows whether things are skipped, so much more reliably than grep!

I propose a CLI command / flag that exits non-zero if .skip() or .only() are used.

ava --check-skips

You would then use that for your pre-push hook. If the tests pass and neither .skip() nor .only() were called, then git will push your commits, otherwise it will display a meaningful message from AVA, even in apps like Tower. All AVA has to do is provide this mode and exit with an error, if appropriate.

An alternative method to enable the mode could be an environment variable, to better support existing npm scripts...

AVA_CHECK_SKIPS=true npm test

If this functionality were to be implemented, we could then automate it across all collaborators on a project with tools like husky. Fewer broken PRs, anyone? :)

[jamestalmage]

See: https://github.com/avajs/eslint-plugin-ava/blob/master/docs/rules/no-skip-test.md

[sholladay]

I do use that a lot in my personal projects. It is definitely better than grep. However, there are circumstances where I cannot use it. In particular, projects that use TypeScript or other transpiled languages, which are popular within companies. AVA works perfectly well in these contexts, whereas ESLint often does not.

Further, I would argue, AVA's runner should be more robust than static analysis.

[sindresorhus]

The ESLint rules have saved me many many times. I do understand not everyone will or can use that though, so I'm open to doing something in AVA. It's a very common mistake to make.

Maybe we could also have an option to fail if .only() is used in CI.

@novemberborn Thoughts?

[novemberborn]

Definitely 👍 on doing this by default in CI.

I suppose --check-skip could have a corresponding checkSkip configuration option, and if that's defined and false, this could override the CI default for those who still don't want the failures.

I'm not keen on supporting configuration through environment variables. If we'd support a JS file to build the configuration though that could be implemented locally.

[sholladay]

I'm not keen on supporting configuration through environment variables.

I hear that. I usually go to great lengths to avoid env vars. In this case, though, it would make it easier to have hooks that make no assumptions about the npm test command. I thought about npm test -- --check-skips as an alternative, but that would only work if ava was the last command used. That's usually the case for me, but not always, and making that assumption is going too far IMO.

If we'd support a JS file to build the configuration though that could be implemented locally.

Hmm well to me this isn't really any different than just having my pre-push hook modify package.json momentarily to add "ava" : { "skipChecks" : true }. The idea of doing that kind of scares me. What would a JS file do better exactly? I guess you were thinking it could do if (isRunningInGitHook) { ... } somehow?

[novemberborn]

What would a JS file do better exactly? I guess you were thinking it could do if (isRunningInGitHook) { ... } somehow?

Yes. You could make up your own env var for that, too.

That might be a better place to start than reasoning through which settings should have env vars and which shouldn't.

[revelt]

Just wanted to add a different angle on this issue.

I'm using eslint-plugin-ava and I've got a setup to automatically fix the "error" - to automatically remove the .onlyies or .skips. I then use Atom and set its ESLint plugin to stop autofixing this rule. Sadly, VSCode Eslint plugin doesn't have such feature, it's impossible to whitelist auto-fixing in ESLint.

Now, if somebody would port the .only/.skip prevention mechanism to native AVA, it would allow users to migrate from Atom to VSCode. We would stop using linter's rule ava/no-only-test and CI would recognise them and fail the build.

For the record, failing CI builds are nasty, a waste of time. Here's an alternative idea.

What if we introduced the -dev / -d flag on ava's CLI?

We could piggyback many features on dev mode:

• allowing .only/.skip to be present in this mode and only in this mode, otherwise ignoring them both natively by AVA
• pre-filtering all globbed test files and look for .only in any of them, if found, isolate that file (it would slow down ava) (Would solve #1472)
• we could implement a test file isolation which would work in this mode only. I have such feature set in my local AVA's fork, I put string avaonly in any of test files and that file is isolated.

For posterity, #1472 and per-file isolation is just a single .then filtering globby's result, but it is best ran in "dev" mode, just like you see below:

const files = await globby(patterns, {
    absolute: true,
    braceExpansion: true,
    caseSensitiveMatch: false,
    cwd,
    dot: false,
    expandDirectories: false,
    extglob: true,
    followSymbolicLinks: true,
    gitignore: false,
    globstar: true,
    ignore: defaultIgnorePatterns,
    baseNameMatch: false,
    onlyFiles: true,
    stats: false,
    unique: true
  }).then(filesArr => {
    if (dev && Array.isArray(filesArr) && filesArr.length > 1) {
      for (let i = filesArr.length; i--; ) {
        const filesContents = fs.readFileSync(filesArr[i], "utf8");
        if (
          filesContents.includes(".only(") ||
          filesContents.includes("avaonly")
        ) {
          return [filesArr[i]];
        }
      }
      return filesArr;
    }
    return filesArr;
  });

What do you think?

[sholladay]

I wouldn't want AVA to just ignore the skips, ever. It should either respect them or demand they be removed. That ensures good code hygiene.

Other than that, I think the idea of a dev mode is fine.

[novemberborn]

Without more use-cases for a --dev flag I don't want to include that just yet. Re-reading this conversation we're open to having a --check-skips flag / option, defaulting to true when CI is detected.

I don't think we quite spelled it out, so I think this should cause failures when .only() and .skip() are used, for tests, and also when individual assertions are skipped.