Stale "Tests with previous failures not run" when changing source file

[IgnusG]

I noticed that changing the source file sometimes leads to ava reporting a "1 previous failure in test files that were not rerun" even though the tests testing that file already passed. This message is a bit confusing since the test actually was rerun and passed.

I saw that the relevant line that "prunes" the failures is

ava/lib/watcher.js

Line 267 in 9cedc97

this.pruneFailures(plan.files);

But that line seems to run after the failures have already been passed to the api at

ava/lib/watcher.js

Line 125 in 9cedc97

previousFailures: this.sumPreviousFailures(this.runVector),

and it's therefore not up to date anymore (since we overwrite the array in prune - not modify it).

It's either possible to modify the array instead at

ava/lib/watcher.js

Line 293 in 9cedc97

this.filesWithFailures = this.filesWithFailures.filter(state => !toPrune.has(state.file));

or to pass it as a function that gets evaluated instead:

watcher.js

runtimeOptions: {
  clearLogOnNextRun,
  previousFailures: () => this.sumPreviousFailures(this.runVector), // This is now a function
  runOnlyExclusive,
  runVector: this.runVector,
  updateSnapshots: updateSnapshots === true
}

and reporters/default.js

if (this.previousFailures() > 0) { // Which we call in the reporter to get the up to date data
  this.lineWriter.writeLine(colors.error(`${this.previousFailures()} previous ${plur('failure', this.previousFailures())} in test files that were not rerun`));
}

The code is taken from the latest commit on ava@next

[bobthekingofegypt]

I think this is the same issue being reported in #2040. This post breaks down the flow really well.

@panjiesw posted up a sample repo and instructions for how to replicate it.

I had a look at what was happening just to check it wasn't related to my original fix.
The problem with the approach suggested above is it will break the times where this is the desired behaviour, if you have two test files in failure and you fix one file the message should still be "x passed, x failed in other file not rerun". These TAP tests fail with the fix.

I think the issue can be solved by something like bobthekingofegypt@f823a62. Or just setting previous failures array to the empty array when run all is triggered. It's confusing following the watcher logic though so I'm not sure of any side effects that the test cases aren't covering.

[novemberborn]

It's confusing following the watcher logic though so I'm not sure of any side effects that the test cases aren't covering.

Yea it's pretty old logic by now, and the tests use too many stubs. I was going to say that I think the test cases are fairly comprehensive but clearly there's a bug so I guess not 😄

I think we can make the change and I'll double check it in the PR. And separately do more of an integration-style test using the newer testing approach in test/.

[bobthekingofegypt]

Do you want me to open that branch as a pull request? It was just there to help the conversation but happy to create a pull request for it.

[novemberborn]

@bobthekingofegypt yes that'd be useful.

[yurivish]

Out of curiosity, is the fact that this issue was closed as “not planned” indicative of the fact that it is fixed? I think I ran into it last year.

[novemberborn]

I don't know, might be better in the main branch / next release.