"previous failure in test files that were not rerun" not always true with ava --watch

mtfurlan commented 5 years ago

Description

If you have a failing test, and you change it to passing, ava --watch will report tests passed, but also 1 previous failure in test files that were not rerun

Test Source

'use strict';
const test = require('ava');
test('Tests run', async t => {
    t.fail();
});

Steps to reproduce

Make a new dir.
Put the test source in test/test.js

Run

npm i ava
./node_modules/.bin/ava test/** --watch

Edit the test to be t.pass()

The output will be

  1 test passed [15:55:38]
  1 previous failure in test files that were not rerun

Steps to reproduce

Expected output:

  1 test passed [15:55:38]

As there is only one test, and it is now passing, there should be no previous failures that aren't re-run.

Environment

mark@boethiah(s):/tmp/ava-testing-test$ node -e "var os=require('os');console.log('Node.js ' + process.version + '\n' + os.platform() + ' ' + os.release())"
Node.js v10.15.1
linux 4.9.0-8-amd64
mark@boethiah(s):/tmp/ava-testing-test$ ./node_modules/.bin/ava --version
1.2.1
mark@boethiah(s):/tmp/ava-testing-test$ npm --version
6.4.1

IssueHunt Summary

#### [

bobthekingofegypt](https://issuehunt.io/u/bobthekingofegypt) has been rewarded. ### Backers (Total: $60.00) - [

issuehunt](https://issuehunt.io/u/issuehunt) ($60.00) ### Submitted pull Requests - [#2192 Consistent paths in watcher](https://issuehunt.io/r/avajs/ava/pull/2192) --- ### Tips - Checkout the [Issuehunt explorer](https://issuehunt.io/r/avajs/ava/) to discover more funded issues. - Need some help from other developers? [Add your repositories](https://issuehunt.io/r/new) on IssueHunt to raise funds. --- IssueHunt has been backed by the following sponsors. [Become a sponsor](https://issuehunt.io/membership/members)

novemberborn commented 5 years ago

Thanks for reporting this @mtfurlan. I've also seen this happen.

IssueHuntBot commented 5 years ago

@issuehunt has funded $60.00 to this issue.

Submit pull request via IssueHunt to receive this reward.
Want to contribute? Chip in to this issue via IssueHunt.
Checkout the IssueHunt Issue Explorer to see more funded issues.
Need help from developers? Add your repository on IssueHunt to raise funds.

bobthekingofegypt commented 5 years ago

I have been taking a look at this.

I think I have fixed the issue, and I have got the integration tests to run through, I need to get my head around the normal watcher tests now. Will have another look at the weekend.

The issue seems to be with absolute and relative paths, there is a mixture in use throughout the watcher so the comparison with pruneFailures sometimes fails depending on where the event comes from.

novemberborn commented 5 years ago

I think I have fixed the issue, and I have got the integration tests to run through, I need to get my head around the normal watcher tests now. Will have another look at the weekend.

That's great!

The watcher tests are… a bit much. I wonder what our code coverage is if we just run the integration tests.

The issue seems to be with absolute and relative paths, there is a mixture in use throughout the watcher so the comparison with pruneFailures sometimes fails depending on where the event comes from.

That sounds very plausible, nice find.

bobthekingofegypt commented 5 years ago

I believe this is the same issue as #2069.

I got the existing test suite to run now, in the unit tests basically any call to change needs to be a relative path and everything else should be absolute to match the real behaviour of chokidar. May be easier in future to move chokidar up a level and replace it with a wrapper that transparently converts to absolute path so this unit test group doesn't have to worry about the distinction.

What I haven't done yet is write a test case to validate this fix. Should be able to write one of these complicated unit tests to recreate the original issue, will try it when I get some time.

The integration test will be hard though. One possible way would be create a temp file with a fail call and replace it with a passing version (those two versions being in the integration test set but copied to different location by the test). But I'm not sure if you want file creation and cleanup to become part of your integration test suite?

novemberborn commented 5 years ago

I believe this is the same issue as #2069.

Oh could be! I've added a to-do item to your PR to double-check.

The integration test will be hard though. One possible way would be create a temp file with a fail call and replace it with a passing version (those two versions being in the integration test set but copied to different location by the test). But I'm not sure if you want file creation and cleanup to become part of your integration test suite?

We do that already I think (touching files, at least). But if you've managed to fix up the current tests then there's no rush.

bobthekingofegypt commented 5 years ago

I have the integration and unit tests passing and I added a unit test that would fail in the old code.

The CI appears to have failed on api tests though, I'm not sure if that's something that normally happens or if I've somehow affected them?

Touching files isn't a problem, creating and possible leaving around temp files if tests break or people abort can be annoying though.

issuehunt-oss[bot] commented 4 years ago

@sindresorhus has rewarded $54.00 to @bobthekingofegypt. See it on IssueHunt

:moneybag: Total deposit: $60.00
:tada: Repository reward(0%): $0.00
:wrench: Service fee(10%): $6.00

alexslade commented 3 years ago

I think this might have regressed, I'm seeing this issue in v3.15.0. cc @novemberborn

./node_modules/.bin/ava --version
=> 3.15.0

./node_modules/.bin/ava --watch
...
# Break a test
1 test failed [20:57:17]
# Fix the test
4 tests passed [20:57:21]
1 previous failure in test files that were not rerun

(Node v14.4.0)

novemberborn commented 3 years ago

@alexslade any chance you can reproduce it in the latest v4 release?

panjiesw commented 3 years ago

@novemberborn encountered the same issue with v3.15.0, upgraded to v4.0.0-alpha2 and the issue is still there. Will provide more reproducible case later, just want to report it for now.

novemberborn commented 3 years ago

@panjiesw is it the same as https://github.com/avajs/ava/issues/2821?

panjiesw commented 3 years ago

@novemberborn yup, seems like the same issue

bobthekingofegypt commented 3 years ago

@panjiesw Do you mind saying which editor and OS you are using?

I had a quick look at this but I don't think its a regression from the original issue I fixed, which was to do with absolute and relative paths.

I can recreate this issue being described occasionally, it happens because vim sometimes writes a temp file called 4913 (to see if it has write permissions on the directory) and then deletes the file. Sometimes chokidar picks this creation up and ava gets a little confused. So I'm not sure if you are experiencing a similar sort of file saving event and ava is just not dealing with it very well.

panjiesw commented 3 years ago

Sorry @bobthekingofegypt for the late update, I used VSCode in Linux machine (Manjaro OS, kernel 4.14).

bob-pasabi commented 3 years ago

I just download VSCode and tried changing a test from pass to fail and back a few hundred times but couldn't recreate the issue. I have an ionotify script watching the folder and don't see VSCode doing anything weird with tmp files unfortunately.

If you run watch mode with the DEBUG=* env variable it should print out the files it is detecting changes in, you could see if it logs anything interesting before you start to see the issue.

(this was my work account by mistake - bobthekingofegypt)

panjiesw commented 3 years ago

@bobthekingofegypt I created a repo to reproduce the issue here: https://github.com/panjiesw/ava-issue

EDIT: use ava@4.0.0-alpha.2

Tried to closely simulate what I experienced in my other projects:

TypeScript + ts-node + tsconfig-paths
ava@3.15.0 and ava@4.0.0-alpha.2
yarn

The key is, as shown in my repo above, it began from failed tests:

file index.ts purposely doesn't expose isOdd
file index.spec.ts purposely use isOdd in test
run yarn test:watch and it will correctly report error as in screenshot below

2021-10-01-211030_4480x1440_scrot

After that, try to expose isOdd in index.ts and save. Now all 2 tests passed, but with message like in the screenshot below

2021-10-01-211152_4480x1440_scrot

But if you re-run the test by typing r and enter, or add more tests, or modify any source files again, it will display success without those extra message

2021-10-01-211245_4480x1440_scrot

I don't know if this the intentional behavior, or if it is the same bug mentioned here and in #2821, or a different bug. If it's intentionally like that, I apologize

avajs / ava