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.

Sign up for GitHub

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

Test with all supported node versions #621

Closed
wants to merge 5 commits into from
Closed

Test with all supported node versions #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
145591e
Test with all supported node versions
rotu Jun 14, 2024
aef3f77
Update CI.yml
rotu Jun 14, 2024
8a47ac9
also use a spread of pnpm versions
rotu Jun 14, 2024
933ceac
wip
rotu Jun 15, 2024
5a265af
remove pnpm engine constraint
rotu Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ jobs:
Build:
if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }}
runs-on: ${{ matrix.os }}
container: ${{ matrix.docker }}
strategy:
fail-fast: false
matrix:
os:
# - ubuntu-22.04
- ubuntu-20.04
- windows-2022
node_version:
- 18
node_version: [10, 12, 14, 16, 18, 20, 22]
Copy link
Member

@aminya aminya Jun 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather only smoke test the older versions. The scripts and tests are for the developers of this repository who have a controlled environment with access to the latest tools.

So, trying to run these for such old Node versions doesn't really help.

For smoke testing, we can have a setup-node at the end of the pipeline install older node versions and just require zeromq to make sure it loads. The build and testing will be done with the latest Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not expecting CI to pass on these versions - I think we're on reasonably the same page.

Can you please fill in the blanks here?

  • When you develop this package, it is okay to demand Node ___ and pnpm ___. (I think 22/9 is perfectly reasonable)
  • When you consume this package, we expect you to be running Node ___ with package manager ___. (I think 18 / npm, pnpm, yarn berry)
  • When you consume the types of this package, we expect TypeScript ___. (I think >=4.7.4, following the lead of typescript-eslint and DefinitelyTyped.

Copy link
Member

Choose a reason for hiding this comment

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

These are good questions. I'd say whatever is defined in CI, is the minimum requirement for the developers, and we aim to support as many as wide Nodejs versions as possible. However, it is not always practical.

So to find the range of Nodejs versions that can consume this project, we can add the above-mentioned smoke tests.

We already have compact tests for TypeScript:

zeromq.js/test/unit/typings-compatibility-test.ts

Line 27 in 828ed28

{version: "4.x", minTarget: "es3"},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say whatever is defined in CI, is the minimum requirement for the developers, and we aim to support as many as wide Nodejs versions as possible. However, it is not always practical.
So to find the range of Nodejs versions that can consume this project, we can add the above-mentioned smoke tests.

I'm not asking what versions can consume this library per CI - as of recently, this library was completely broken in all of its CI. I'm asking what versions should we fix existing tests and set up smoke tests for?

  • I'm asking what versions, if broken, would we regard as a problem?
  • Even that typings compatibility test is maybe not so clear. By setting the version to 4.x I think that means only [email protected] is getting checked by CI, but your intention could have been that we support typescript as far back as 4.0. What version of TypeScript do you mean to target and why?

node_arch:
- x64
cpp_arch:
Expand All @@ -28,10 +28,16 @@ jobs:
- false
docker:
- ""
docker_cmd:
- ""

include:
# https://pnpm.io/installation#compatibility
- {node_version: 10, pnpm_version: 6}
- {node_version: 12, pnpm_version: 6}
- {node_version: 14, pnpm_version: 7}
- {node_version: 16, pnpm_version: 8}
- {node_version: 18, pnpm_version: 9}
- {node_version: 20, pnpm_version: 9}
- {node_version: 22, pnpm_version: 9}
- os: windows-2022
node_version: 18
node_arch: x86
Expand Down Expand Up @@ -63,10 +69,6 @@ jobs:
# Alpine
- os: ubuntu-22.04
docker: node:18-alpine
docker_cmd:
apk add --no-cache pkgconfig curl tar python3 make gcc g++ cmake
musl-dev && npm i -g pnpm && pnpm install && pnpm run
build.prebuild
node_version: 18
node_arch: x64
ARCH: x64
Expand Down Expand Up @@ -105,7 +107,7 @@ jobs:
- uses: pnpm/action-setup@v4
if: ${{ !matrix.docker }}
with:
version: 9
version: ${{ matrix.pnpm_version }}

- name: Install Node
if: ${{ !matrix.docker }}
Expand Down Expand Up @@ -139,10 +141,8 @@ jobs:
- name: Prebuild Docker
if: ${{ matrix.docker }}
run: |
docker login -u ${{ github.actor }} -p ${{ secrets.GITHUB_TOKEN }} ghcr.io
docker pull ${{ matrix.docker }}
docker tag ${{ matrix.docker }} builder
docker run --volume ${{ github.workspace }}:/app --workdir /app --privileged builder sh -c "${{ matrix.docker_cmd }}"
apk add --no-cache pkgconfig curl tar python3 make gcc g++ cmake
musl-dev && npm i -g pnpm@${{ matrix.pnpm_version }} && pnpm install && pnpm run build.prebuild

- name: Upload artifacts
uses: actions/upload-artifact@v3
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
},
"engines": {
"node": ">= 10.2",
"pnpm": ">= 9"
"pnpm": "*"
},
"files": [
"CHANGELOG.md",
Expand Down
Loading