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

fix: Ignore transient tap test directories #350

Closed
wants to merge 6 commits into from
Closed

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 6, 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

@rotu rotu requested a review from a team as a code owner September 6, 2023 23:34
@rotu rotu mentioned this pull request Sep 6, 2023
1 task
@rotu rotu changed the title Ignore tap-testdir-*/ fix: Ignore tap-testdir-*/ Sep 6, 2023
@@ -155,7 +155,9 @@ module.exports = {
'/LICENSE*',
'/CHANGELOG*',
],
ignorePaths: [],
ignorePaths: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @lukekarrys weigh in here since it's surprising to me coming in fresh on this that we don't have anything in here already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rotu the current pattern is to put these paths in lib/content/gitignore and leave ignorePaths as a config to be set by consumers. can you make that change? apologies that this is not discoverable! 🙇🏼

Copy link
Contributor Author

@rotu rotu Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. I can add the breadcrumbs that would have retroactively helped me. The lib/content/gitignore file is clearly a template of some sort. What instantiates this template?

EDIT: nvm found it: https://github.com/npm/template-oss/blob/2a5e18677f6de45d903b7d06ec7780c7d31b6963/lib/util/template.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, any reason why allowPaths and distPaths are inlined in this file but ignorePaths is not?

wraithgar
wraithgar previously approved these changes Sep 8, 2023
@wraithgar
Copy link
Member

CI is goofy cause of a new timers/promises require, don't worry about it.

@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2023

CI is goofy cause of a new timers/promises require, don't worry about it.

That only explains the node@14 errors.

This is curious to me: https://github.com/npm/template-oss/actions/runs/6103503295/job/16623491287?pr=350

Run npm i --prefer-online --no-fund --no-audit -g npm@latest
npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Not compatible with your version of node/npm: [email protected]
npm ERR! notsup Required: {"node":"^18.17.0 || >=20.5.0"}
npm ERR! notsup Actual: {"npm":"8.6.0","node":"v18.0.0"}

I don't expect this since:

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.

@wraithgar
Copy link
Member

npm has a special check it does if it detects you are installing npm globally, strict engines checks are done then. This started in npm 7

cf npm/cli#3731

@wraithgar
Copy link
Member

This is how we've been fixing the ci for packages that didn't get an engines bump to match npm 10 npm/read-package-json#190

@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2023

That's a major wtf! There should definitely more specific error, especially since the error is the exact same one packages can no longer enforce:
https://docs.npmjs.com/cli/v6/configuring-npm/package-json?v=true#enginestrict

@wraithgar
Copy link
Member

Yeah after many rfc calls it was decided that package maintainers should not be able to specify things like that, only the consumers.

npm is a special snowflake because it IS what you use to install packages, so if it ends up installing a version that doesn't work in your current node, you can't run it to fix the problem.

@rotu
Copy link
Contributor Author

rotu commented Sep 12, 2023

Back to a draft to figure out interaction with tapjs/tapjs@a5dc854

@rotu rotu mentioned this pull request Sep 14, 2023
1 task
@rotu rotu changed the title fix: Ignore tap-testdir-*/ fix: Ignore transient tap test directories Sep 14, 2023
@rotu rotu marked this pull request as ready for review September 14, 2023 17:59
lib/content/index.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
workspace/test-workspace/package.json Outdated Show resolved Hide resolved
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh I'm gonna miss using git status to find where TAP_SAVE_FIXTURE left its stuff for a given test but this is the right choice.

@wraithgar
Copy link
Member

@rotu I can't land this cause of conflicts, and they're the kind I can't even resolve in the GitHub ui :/

@lukekarrys
Copy link
Contributor

@rotu can you try fetching the latest origin/main and then running git rebase origin/main on your feature branch? That's what I normally do when conflicts get in a weird state.

@wraithgar
Copy link
Member

Rebased in #364 once @lukekarrys approves it'll auto merge.

wraithgar added a commit that referenced this pull request 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]>
@wraithgar
Copy link
Member

Sorry about the merge conflicts @rotu, we got there in the end. #364 landed.

Thanks again for all the help.

@wraithgar wraithgar closed this Sep 15, 2023
@rotu
Copy link
Contributor Author

rotu commented Sep 15, 2023

Oops! I was going to do that, but thank you for the favor, @wraithgar!

@rotu rotu deleted the silent-vicuna branch September 15, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] lint script broken [BUG] tap-testdir-* is not gitignored
3 participants