Removes the redundant JSON array parsing. #652
Conversation
JSON is a subset of YAML, making it redundant.
Makes tools into a function to be able to mock it. It does not need to be exported.
There was a problem hiding this comment.
Pull Request Overview
This PR adds Jest-based unit testing infrastructure to the project and modifies the toolkit initialization to be lazy-loaded. The changes enable proper testing of utility functions that previously required GitHub Actions environment variables at module load time.
Key changes:
- Converted toolkit initialization from eager to lazy loading to support testing
- Removed redundant JSON parsing step from
parseInputArray(YAML parser handles JSON) - Added Jest testing framework with configuration and CI integration
Reviewed Changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Changed rootDir to "." to support compiling test files |
| src/util.ts | Refactored toolkit to lazy initialization and simplified parseInputArray to only use YAML parser |
| test/util.test.ts | Added unit tests for parseInputArray function with environment setup |
| package.json | Added jest dependencies and updated test scripts |
| jest.config.js | Added Jest configuration for ts-jest |
| .husky/pre-commit | Added test execution to pre-commit hook |
| .github/workflows/test.yml | Added CI test job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughAdds Jest-based tests and CI Test job, updates pre-commit to run tests, introduces jest.config.js, refactors src/util.ts for lazy Toolkit initialization and removes JSON parsing in parseInputArray, adds tests for parseInputArray, and adjusts tsconfig rootDir. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as caller
participant Util as src/util.ts
participant Toolkit as Toolkit (init)
rect rgb(240,248,255)
note right of Util: getToolkit lazy-init flow
Caller->>Util: call getToolkit() / getUserInfo()
alt tools undefined
Util->>Toolkit: new Toolkit(...)
Toolkit-->>Util: Toolkit instance
Util-->>Caller: return tools
else tools defined
Util-->>Caller: return existing tools
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
23-27: Update action versions for consistency.The Test job uses older action versions (v4) while the Build and Lint jobs use newer versions (v5/v6). This inconsistency can lead to unexpected behavior and maintenance overhead.
Apply this diff to align with the other jobs:
- - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@v5 + - uses: actions/setup-node@v6 with: cache: npm
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/test.yml(1 hunks).husky/pre-commit(1 hunks)jest.config.js(1 hunks)package.json(2 hunks)src/util.ts(2 hunks)test/util.test.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/util.test.ts (1)
src/util.ts (1)
parseInputArray(83-95)
src/util.ts (1)
src/io.ts (2)
input(25-25)output(35-35)
🪛 actionlint (1.7.8)
.github/workflows/test.yml
21-21: label "ubuntu-20.04" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-11-arm", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🔇 Additional comments (9)
tsconfig.json (1)
4-4: LGTM! Root directory change enables test compilation.The change from
"./src"to"."allows TypeScript to properly compile test files alongside source files, which is necessary for the Jest testing infrastructure being introduced.package.json (2)
12-12: LGTM! Test script properly chains Jest and linting.The test script correctly runs Jest tests followed by linting, ensuring code quality checks happen after functional tests pass.
24-34: LGTM! Jest dependencies are appropriate.All necessary dependencies for Jest with TypeScript support are correctly added:
jestand@types/jestfor the test frameworkts-jestfor TypeScript transformationeslint-plugin-prettierfor code formatting.husky/pre-commit (1)
1-3: LGTM! Pre-commit hook now validates tests before build.Running tests before building ensures that broken code cannot be committed, improving code quality and reducing CI failures.
test/util.test.ts (2)
4-16: LGTM! Environment setup ensures Toolkit initialization works.The environment variable setup in
beforeAllprovides the required GitHub context variables that the Toolkit needs, preventing initialization errors during tests.
18-32: LGTM! Comprehensive test coverage for parseInputArray.The test suite effectively covers all critical paths:
- Valid YAML/JSON string arrays
- Single string inputs (fallback behavior)
- Non-string elements (type validation)
- Invalid input (error handling)
This validates the YAML-only parsing approach and confirms JSON arrays work through YAML parsing.
jest.config.js (1)
1-5: LGTM! Jest configuration is appropriate.The minimal configuration with
ts-jestpreset andnodeenvironment is perfect for testing GitHub Actions TypeScript code.src/util.ts (2)
9-23: LGTM! Lazy initialization prevents import side effects.The lazy initialization pattern through
getToolkit()is a good improvement that avoids side effects during module import, making tests cleaner and stack traces more coherent (as discussed in past review comments).
80-95: LGTM! Redundant JSON parsing successfully removed.The removal of the separate JSON parsing path is correct because YAML is a superset of JSON, so
YAML.load()can parse JSON arrays natively. The test suite confirms this works correctly for:
- JSON arrays:
["a", "bcd"]- Single strings:
"hello"(falls back to single-element array)- Non-string elements:
[42](falls back to single-element array)- Invalid input:
"(falls back to single-element array)This change fulfills the PR objective of removing redundant JSON parsing while maintaining all necessary functionality.
|
@all-contributors please add @tommie for their code and test contribution |
|
I've put up a pull request to add @tommie! 🎉 |
2 participants
Adds Jest as a test framework and tests for JSON parsing.
Closes #646.
Summary by CodeRabbit
Tests
Chores
Refactor