Skip to content
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

[BUG] tap-testdir-* is not gitignored #348

Closed
1 task done
rotu opened this issue Sep 5, 2023 · 6 comments · Fixed by #364
Closed
1 task done

[BUG] tap-testdir-* is not gitignored #348

rotu opened this issue Sep 5, 2023 · 6 comments · Fixed by #364
Labels
Needs Triage needs an initial review

Comments

@rotu
Copy link
Contributor

rotu commented Sep 5, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

When running npm/cli tests on windows, I was seeing many files churn in and out of the source control files list.

These ephemeral test files slow down the IDE considerably and can make it hard to see what actually changed.

image

Expected Behavior

Files created by tap tests should either be:

  1. ignored by gitignore.
  2. created outside the git working tree

Steps To Reproduce

  1. In VSCode on Windows, clone https://github.com/npm/cli
  2. Open the Source Control pane
  3. Run npm test

Environment

  • npm: 9.6.7
  • Node: 18.17.1
  • OS: Windows 11
  • platform: AMD64 Desktop
@rotu rotu added the Needs Triage needs an initial review label Sep 5, 2023
@rotu
Copy link
Contributor Author

rotu commented Sep 5, 2023

Note this seems to happen in GitHub codespaces as well.

@lukekarrys
Copy link
Contributor

One reason that those aren't currently ignored is that they are ephemeral and should be deleted by the end of npm test in CI. We then have git status checks which would fail if those files are not properly cleaned up. I'm not currently sure what situation is testing against though. It might be safe to assume those files are always cleaned up and if they are not then it's a problem with our testing framework

And you make a good point about making it difficult to see what has changed and causing contributor friction. I'd be in favor of this change.

@rotu would you like to make a PR for this?

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2023

I gave it a shot in #350.

Note that terminating tests with ctrl-c also results in these files not getting cleaned up, which compounds the problem (not sure if on purpose or due to npm/cli#6766).

@wraithgar
Copy link
Member

In my current setup I do rely on those files showing up in git status when I want to tell tap to keep fixtures, so I can find them easily. It's not a deal breaker but it'll be mildly annoying to have to try to hunt them down if git is ignoring them.

@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2023

In my current setup I do rely on those files showing up in git status when I want to tell tap to keep fixtures, so I can find them easily. It's not a deal breaker but it'll be mildly annoying to have to try to hunt them down if git is ignoring them.

git status --ignored ./test

It does seem odd to me that the snapshots are centralized under a tap-snapshots/ folder, but the fixtures are not under some tap-testdir/ root. I would feel more comfortable avoiding globs in .gitignore where possible, and I would think any tooling that watches the file system would prefer it too!

@wraithgar
Copy link
Member

Yeah I'll probably have to finally commig --ignored to memory. This is not a deal breaker to be sure.

As to where tap puts things, that's something to bring up in the tapjs repos. I know Isaac is hard at work on the next major version right now.

wraithgar added a commit that referenced this issue Sep 15, 2023
Running tests causes a lot of file churn. Although (usually) ephemeral,
these files cause high CPU usage and can make developer tools difficult
to use. These files also make linting fail.

Now the files will be ignored by git and by eslint.

## References
Fixes #348
Fixes #359

This is a rebase of #350 since it had yet another conflict.

---------

Authored-by: Dan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Triage needs an initial review
Projects
None yet
3 participants