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 test of silent exit on unsupported node.js versions #1829

Merged
merged 5 commits into from Jul 19, 2022

Conversation

voxpelli
Copy link
Member

@voxpelli voxpelli commented Jul 17, 2022

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix
[ ] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

  • Used version-guard for the silent exit (this helped identify a mismatch in the version set in bin/cmd.js and the one in package.json)
  • Used .cjs file ending to avoid having to resort to eval()
  • Gathered all non-index.js files (since there's now a cli.js file as well) in lib/
  • Added Node.js 4 to old-test.yml, to get a a signal of how far back any possible breakage goes
  • Stopped caching dependencies as we don't really want these very old npm versions to mess around with the cache that's shared by the rest
  • Stopped installing dev dependencies in old-test-yml to avoid problems on npm install (this fixed the tests)

Which issue (if any) does this pull request address?

Closes #1828

Is there anything you'd like reviewers to focus on?

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

Thanks for investigating how to fix the old tests on master.
Some comments need more clarification of what's going on.

While I still think, we should not do some "version guard" things, I don't mind for the moment if we continue to fail silently for the older Node.js versions. About that, I will continue to discuss that in #1828, so this PR only talks about the implementation, not the possible solutions. 😄

.github/workflows/old-test.yml Outdated Show resolved Hide resolved
bin/cmd.cjs Show resolved Hide resolved
@voxpelli voxpelli force-pushed the fix-test-of-silent-exit-on-unsupported branch from c36babb to f635e4e Compare July 18, 2022 15:22
@voxpelli voxpelli requested a review from theoludwig July 18, 2022 15:25
Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

LGTM!

@voxpelli voxpelli merged commit 6dd2162 into master Jul 19, 2022
@voxpelli voxpelli deleted the fix-test-of-silent-exit-on-unsupported branch July 19, 2022 10:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Old test is failing on master
2 participants