Conversation
adespawn
force-pushed
the
extended-ci
branch
3 times, most recently
from
7d97bb0 to
e5eea2c
Compare
| #### Integration tests | ||
|
|
||
| | | Linux x64 | Linux arm**| MacOS Intel | MacOS Arm | | ||
| |-------------- |----------- |----------- |------------- |----------- | | ||
| | Node 20 | ✅ | ❌ | ❌ (planned) | ❌ | | ||
| | Node 22 | 🟠 | ❌ | ❌ (planned) | ❌ | | ||
| | Node 24 | 🟠 | ❌ | ❌ (planned) | ❌ | | ||
| | Node current | 🟠 | ❌ | ❌ (planned) | ❌ | |
There was a problem hiding this comment.
💭 Shouldn't we rather test newest Node.js in the main CI to discover problems with new Node.js implementations quickly? Node 20 should be left in the main CI as well; the remaining Node.js implementations should be run in the extended CI.
There was a problem hiding this comment.
We need to run tests on Linux ARM IMO.
There was a problem hiding this comment.
Shouldn't we rather test newest Node.js in the main CI to discover problems with new Node.js implementations quickly
I'm on the edge here.
On one had your argument is valid, but on the other hand, when running on newer node version, we may accidently introduces changes that depend on the newer node versions. Also, the legendary memory bug was present on the older node versions - not the newest.
There was a problem hiding this comment.
We need to run tests on Linux ARM IMO.
I had some problems with CCM on arm CI, although I didn't document what specific issue that was.
There was a problem hiding this comment.
Node 20 should be left in the main CI as well
Reading the whole message before replying would be nice. It depends if we want to have 2 integration tests on each PR. It's already the longest CI, and I already added examples on the newest version to the CI, to catch some obvious bugs
There was a problem hiding this comment.
If CI longevity is the problem for the development, then I agree - let's only run integration tests with the oldest supported Node.
There was a problem hiding this comment.
It's more of a concern amount of tasks the CI takes - they will be done in parallel.
There was a problem hiding this comment.
And what's the concern of the amount of tasks about? CI costs?
There was a problem hiding this comment.
When testing #393, I've run into situations where despite running only integration tests CI, some of the run (i've run few in parallel) were waiting for other to finish, before starting. I worry, that when we increase the number or running tasks (especially long one), it may slow down the CI significantly in situations when I push changes to multiple PRs
There was a problem hiding this comment.
Or somehow else trigger CI multiple times
There was a problem hiding this comment.
Pull request overview
Adds an “extended CI” workflow intended to run a broader test matrix on releases, and refactors CI logic into shared composite actions.
Changes:
- Introduces a new tag-triggered “extended CI” workflow covering full examples + integration test matrices.
- Refactors existing workflows to use shared composite actions (
.github/common/setup,example,integration) and trims the regular examples matrix. - Documents the CI split (regular vs extended) and updates the release checklist.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| MAINTENANCE.md | Documents regular vs extended CI and updates release steps. |
| .github/workflows/unit-tests.yml | Switches setup step to shared composite action. |
| .github/workflows/typescript-tests.yml | Switches setup step to shared composite action. |
| .github/workflows/release.yml | Switches setup step to shared composite action. |
| .github/workflows/integration-tests.yml | Replaces inline integration setup with shared composite action. |
| .github/workflows/examples.yml | Reduces regular examples matrix and uses shared example composite action. |
| .github/workflows/benchmarks.yml | Switches setup reference (currently incorrect path). |
| .github/workflows/extended-ci.yml | Adds new workflow for extended examples + integration matrices. |
| .github/common/setup/action.yml | Adds description for shared setup composite action. |
| .github/common/integration/action.yml | Adds shared composite action for integration tests. |
| .github/common/example/action.yml | Adds shared composite action for running examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Moves the setup action to a common folder, so that we can create additional common actions in the future (see next commit). Also update description, as linter complains if this field is missing
| run: docker compose -f .github/docker-compose.yml logs | ||
| shell: bash | ||
| - name: Remove cluster | ||
| if: ${{ always() }} |
There was a problem hiding this comment.
This is missing in Rust Driver's CI, please add it there too.
This commit extracts the logic for running examples and integration tests into a separate reusable GitHub Action. Those actions will be then also used in the extended CI workflow added in the next commit.
This adds an extended CI workflow that runs integration tests and examples on all supported node versions, for all supported architectures.
adespawn
force-pushed
the
extended-ci
branch
from
6d1b823 to
83c5346
Compare
|
All typo comments should be resovled now |
|
@adespawn Is this ready for re-review? |
Yes. The only remaining parts were to discuss two big discussions still open, but with the ZPP chaos on Tuesday I didn't get the change to do it. |
|
Add macos examples as planned, in the future consider some smoke tests for newest node version (for now we have examples, consider if I can do something better, or is it good enough) |
This adds documentation about the existing CI workflows and the split between regular and extended CI. It also update the release instruction, to add the reminder about extended CI.
This was a leftover from the NAPI-RS init template and is not needed for this CI.
The main goal of this PR is to add a new extended version of the CI (as discussed in #292)