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

Jump to bottom

Added GitHub Action workflows to run the tests on various versions of Node.js #126

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from
Open

Added GitHub Action workflows to run the tests on various versions of Node.js #126

wants to merge 31 commits into from

Conversation

jamesmortensen
Copy link

@jamesmortensen jamesmortensen commented Mar 17, 2022

I created two workflow files, one uses the node-setup action to run through all of the versions of Node in the matrix list and run the tests. All tests passed except on v5.x and v0.10.0 due to npm incompatibilities.

nvm has a feature that lets you update npm to the latest known working version with a specific node version. So I created another workflow to use nvm to install various node versions instead of the node-setup action. Specifically for v5.x and v0.10.0, I ran nvm install-latest-npm to update npm to a working version.

This results in tests passing for v5.x, but there are certificate errors when attempting to npm install using Node v0.10.0. You can see the results here: https://github.com/jamesmortensen/module-alias/runs/5581223733?check_suite_focus=true

Not sure how to proceed from here, so I thought I'd share what I'd done so far.

@jamesmortensen
adding workflow to validate v0.10 is available via the node-setup action
f17255e
@jamesmortensen
Fix version number
4a876db
@jamesmortensen
Add workflow dispatch
cb53f89
@jamesmortensen
Remove npm cache
e7ee41c
@jamesmortensen
Removed npm cache
040dc3f
@jamesmortensen
added in versions from the travis ci config, and added in npm test co…
ac95a5a 
…mmand
@jamesmortensen
Run testonly
0291d81
@jamesmortensen
trying with npx as mocha was not found
abc849b
@jamesmortensen
forgot npm ci
a4b43f2
@jamesmortensen
0.10.0 is so old that ci does not yet exist, trying npm install
26b3032
@jamesmortensen
Run all of the jobs even if one fails
33d4d7d
@jamesmortensen
Fix fail fast
49e1f02
@jamesmortensen
Adding in 5.0.0
9bbb59e
@jamesmortensen
Adding workflow to use nvm to install node v5
1040a2d
@jamesmortensen
Rename workflow
81c745d
@jamesmortensen
Install nvm, even though GitHub Action docs say it's already there: h…
f701763 
…ttps://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md
@jamesmortensen
load nvm into shell
ac2c885
@jamesmortensen
Add nvm command to same run group
856880a
@jamesmortensen
Keep NVM_DIR across all run blocks
642efc0
@jamesmortensen
Keep NVM_DIR across all run blocks
8179aea
@jamesmortensen
nvm use in each block and npm install
465b961
@jamesmortensen
put all nvm and node,npm calls in one block
372e885
@jamesmortensen
install latest compatible npm
ef0c013
@jamesmortensen
Add matrix to the nvm workflow
c4f5d35
@jamesmortensen
Handling issues with v0.10.0 being defined with minor and patch versi…
f1f8053 
…ons as arithmetic operations must be done only with major version to determine if we need to upgrade npm
@jamesmortensen
handling the oldest node version. npm must be upgraded more than once.
e87be35
@jamesmortensen
Updated node.js to use latest v5, merged to dev branch. enabled nvm w…
1bb12af 
…orkflow to run on push as well
@jamesmortensen
added nodejs workflow to dev push and pr as well
a711516
@jamesmortensen jamesmortensen mentioned this pull request Mar 17, 2022
Open
@Kehrlann
Copy link
Collaborator

@Kehrlann Kehrlann commented Mar 18, 2022 •
edited

Hey @jamesmortensen 👋

Thanks a lot for contributing, and taking the time to dig into this. My thoughts so far:

  1. I think we're fine with whatever Github gives us as a node version, no need to go the extra mile with the nvm-managed versions. Let's keep the nodejs workflow.
    1. It's equivalent to what we had with Travis CI
    2. And it's much better than what we have now ~ nothing 😅
  2. Let's get this merged asap:
    1. Remove 5.0 and 0.10
    2. Keep the LTS: 6 (it passes locally, I tried), 8, 10, 12, 14, 16
    3. Add 17 (why not?)
  3. Rename the worfklow test
  4. Remove .travis.yml as part of your PR

I think I'm inclined towards cutting a 3.0.0 release, dropping support for older node versions. I don't have the bandwidth to dig into testing issues on node versions that were End-Of-Life'd in 2016. I'll sleep on it.

Also, if you're interested, feel free to open another PR with a lint workflow, that runs lint with the latest stable node version.

@jamesmortensen
Copy link
Author

@jamesmortensen jamesmortensen commented Mar 19, 2022

Ok, @Kehrlann I'll make the changes this next week when I get more time to look at this. For what it's worth, it seems like a hassle to support 0.10.0 and 5.x, so... great call!

James

@jamesmortensen
Copy link
Author

@jamesmortensen jamesmortensen commented Mar 20, 2022

@Kehrlann I made the changes.

  • Travis deleted
  • NVM workflow deleted
  • versions to test are 6, 8, 10, 12, 14, 16, and 17
  • workflow renamed to "test".

Looking forward to the 3.0.0 release!

@jamesmortensen
Copy link
Author

@jamesmortensen jamesmortensen commented Mar 29, 2022

Hi @Kehrlann I just wanted to bump this thread. Are we ready to merge this pull request? Please let me know if there is anything else we need to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jamesmortensen @Kehrlann