chore: migrate travis to github actions #246
Conversation
## Walkthrough
A new GitHub Actions workflow file named `ci.yaml` has been added under the `.github/workflows` directory. This file sets up a continuous integration pipeline for a Node.js project, configured to run on pushes and pull requests to the `master` branch. The workflow tests the project across multiple Node.js versions using a matrix strategy and includes steps for checking out code, setting up Node.js with N|Solid, installing dependencies, and running tests. Additionally, several source files (`commands/report.js`, `lib/ncm-analyze-tree.js`, `lib/report/github-action.js`, and `lib/util.js`) were modified with stylistic changes such as removal of semicolons, commenting out unused variables, and minor import adjustments without altering functionality or exported interfaces. Test files and snapshot outputs were updated to reflect reduced package counts and adjusted risk and vulnerability numbers in reports.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|------------------------------------------------------------------------------------------------------------------|
| .github/workflows/ci.yaml | Added a new GitHub Actions workflow for CI, testing across Node.js versions 18, 20, and 22 with N|Solid on push and PR to `master`. Includes setup, dependency installation, and test execution steps. |
| commands/report.js | Commented out `includedCount` and `skippedCount` variables and related increments; removed trailing semicolons and extra blank lines; no logic changes. |
| lib/ncm-analyze-tree.js | Refactored for style: removed trailing semicolons, extra blank lines, replaced some `let` with `const`, commented out fallback code; no functional changes. |
| lib/report/github-action.js | Removed semicolons from variable declarations and object literals; purely stylistic syntax changes. |
| lib/util.js | Removed `ncmApi` import from destructured `./config` import; no other changes. |
| tap-snapshots/test/report.js.md | Updated snapshot outputs to reflect fewer packages checked (36 to 4), adjusted risk and vulnerability counts, and pruned module listings consistently across all report variants. |
| test/report.js | Updated test assertions to match reduced package counts and adjusted risk/vulnerability numbers; replaced some module/version expectations; no control flow or error handling changes. |
## Poem
> In the warren, code hops anew,
> With workflows set for every view.
> Across Node’s versions, tests all run,
> Ensuring bugs are quickly shunned.
> The CI rabbit’s work is done—
> Now merge with joy, the build has won!
> 🐇✨📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
1-25: Solid migration to GitHub Actions; consider optional improvements.This workflow is a robust replacement for Travis CI, covering a wide range of Node.js versions and ensuring tests run on both pushes and PRs to
master. The use ofnpm ci || npm installis pragmatic, but be aware it may mask lockfile issues—ideally, CI should fail ifnpm cifails due to lockfile drift. You may also want to:
- Add caching for
~/.npmand/ornode_modulesto speed up builds.- Consider supporting
mainbranch or additional branches if your repo naming changes.- Optionally add a linting step (e.g.,
npm run lint) for code style enforcement.- If you want to collect test results or coverage, add steps to upload those as artifacts.
Overall, this is a well-structured and effective CI workflow.
riosje
force-pushed
the
jeff/migratecitoactions
branch
3 times, most recently
from
59cbad0 to
27d8fc3
Compare
Signed-off-by: Jefferson <jefferson.rios.caro@gmail.com> chore: migrate travis to github actions
riosje
force-pushed
the
jeff/migratecitoactions
branch
from
6c783eb to
d98ad25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
1-28: Solid migration to GitHub Actions—consider npm caching for faster buildsThis workflow is well-structured and achieves the goal of migrating CI from Travis to GitHub Actions, covering multiple Node.js/N|Solid versions and ensuring tests run reliably. For further optimization, consider adding npm caching to speed up dependency installation:
- name: Cache npm uses: actions/cache@v4 with: path: ~/.npm key: ${{ runner.os }}-node-${{ matrix.node-version }}-nsolid-${{ matrix.nsolid-version }}-${{ hashFiles('**/package-lock.json') }} restore-keys: | ${{ runner.os }}-node-${{ matrix.node-version }}-nsolid-${{ matrix.nsolid-version }}-Otherwise, this is a strong and maintainable CI setup.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
lib/ncm-analyze-tree.js (5)
326-326: Use optional chaining for safer property access.Static analysis suggests using optional chaining here. If
pkgJsonis potentially undefined or null, usingpkgJson?.nameandpkgJson?.versionwould prevent runtime errors.Apply this diff for safer property access:
- if (pkgJson && pkgJson.name) { - possibleEntryPoints.unshift(`bin/${pkgJson.name}.js`) - possibleEntryPoints.unshift(`${pkgJson.name}.js`) + if (pkgJson?.name) { + possibleEntryPoints.unshift(`bin/${pkgJson.name}.js`) + possibleEntryPoints.unshift(`${pkgJson.name}.js`)🧰 Tools
🪛 Biome (1.9.4)
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
473-475: Add unit tests for uncovered code paths.The CI pipeline reports missing code coverage for several blocks, specifically for:
- Fallback logic in
readUniversalTree(lines 473-475, 487-489)- Adding
devDependenciesandpeerDependenciesinreadPackagesFromPackageJson(lines 517-525, 529-537)Consider adding tests that:
- Trigger the fallback to
readPackagesFromPackageJson(e.g., when no main file or children are found).- Ensure
devDependenciesandpeerDependenciesare parsed and included as expected.Would you like help drafting these tests or opening an issue to track this?
Also applies to: 487-489, 517-525, 529-537
🧰 Tools
🪛 GitHub Actions: N|Solid CI
[error] 473-537: Code coverage missing for lines 473-475,487-489,517-525,529-537.
200-203: Remove or clarify commented-out counters infilterPkgs.There are several commented-out counter variables and increment statements in
filterPkgs. If these are no longer needed, consider removing them for clarity. If you plan to use them for debugging or metrics, add a comment explaining their purpose.Also applies to: 207-207, 213-215, 219-219, 221-221
236-273: Remove or document large commented-out fallback function.The fallback function
readPackagesFromPackageJsonis commented out in favor of the active implementation below. If this code is obsolete, consider removing it. If you intend to keep it for reference, add a comment explaining why.
11-11: Consistent function declarations.There is a mix of arrow functions and traditional function declarations. While both are valid, consider standardizing for consistency unless there is a specific reason for the distinction (e.g., use of
this).Also applies to: 51-51, 89-89, 482-482
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commands/report.js(2 hunks)lib/ncm-analyze-tree.js(9 hunks)lib/report/github-action.js(1 hunks)lib/util.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- lib/util.js
- lib/report/github-action.js
- commands/report.js
🧰 Additional context used
🪛 Biome (1.9.4)
lib/ncm-analyze-tree.js
[error] 326-326: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: N|Solid CI
lib/ncm-analyze-tree.js
[error] 473-537: Code coverage missing for lines 473-475,487-489,517-525,529-537.
|
closing ticket since this must be complete and has been in this pipeline for more than 10 sprints |
Summary by CodeRabbit