ci: Add unit testing infra: #115
Conversation
WalkthroughA new GitHub Actions workflow file named Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
.github/workflows/test.yaml (1)
3-11: Consider enhancing trigger configuration for better efficiency.
- Add the "ready_for_review" type to PR triggers to handle draft PRs properly.
- Consider adding branch filters to the push trigger to avoid unnecessary runs.
on: pull_request: - types: ["opened", "reopened", "synchronize"] + types: ["opened", "reopened", "synchronize", "ready_for_review"] push: + branches: + - main + - develop schedule: # Run at midnight UTC every day with 15 minutes delay added to avoid high load periods - cron: "15 0 * * *"test/utils/math.test.ts (2)
7-27: Consider adding more edge cases to strengthen test coverage.The current test suite provides good basic coverage for the
clampfunction. However, consider adding these scenarios to make it more robust:
- Decimal numbers (e.g.,
clamp(3.14, 1, 5))- Extreme values (e.g.,
Number.MAX_VALUE,Number.MIN_VALUE)- Invalid inputs (e.g.,
NaN,undefined)- Equal boundaries (e.g.,
clamp(3, 5, 5))test("handles decimal numbers correctly", () => { expect(clamp(3.14, 1, 5)).toBe(3.14); expect(clamp(0.5, 1, 5)).toBe(1); expect(clamp(5.5, 1, 5)).toBe(5); }); test("handles edge cases correctly", () => { expect(clamp(3, 5, 5)).toBe(5); // Equal boundaries expect(clamp(NaN, 1, 5)).toBe(1); // Invalid input expect(clamp(Number.MAX_VALUE, 1, 5)).toBe(5); // Extreme value });
29-48: Consider adding boundary test cases for chunk calculations.The current test suite provides good coverage for standard cases. To ensure robust error handling, consider adding these scenarios:
- Very large numbers to verify no integer overflow
- Invalid chunk sizes (negative or zero)
- Edge case chunk sizes (1 or very large numbers)
test("handles invalid chunk sizes", () => { expect(() => getChunkNum(10, 0)).toThrow(); // Should throw for zero chunk size expect(() => getChunkNum(10, -1)).toThrow(); // Should throw for negative chunk size }); test("handles extreme values correctly", () => { expect(getChunkNum(1000000, 1)).toBe(1000000); // Large result expect(getChunkNum(10, 1)).toBe(10); // Minimum valid chunk size expect(getChunkNum(Number.MAX_SAFE_INTEGER, 1000)).toBe(9007199254740); // Very large input });jest.config.ts (3)
13-14: Consider removing or conditionally enabling the console.log statementThe console.log statement might pollute the test output. Consider either removing it or wrapping it in a debug flag condition.
-console.log(`Environment variable "CI"="${process.env.CI}": ` + - `primary reporter will be ${JSON.stringify(PRIMARY_REPORTER)}`); +if (process.env.DEBUG) { + console.log(`Environment variable "CI"="${process.env.CI}": ` + + `primary reporter will be ${JSON.stringify(PRIMARY_REPORTER)}`); +}
62-63: Consider limiting maxWorkers in CI environmentSetting maxWorkers to "100%" might overwhelm CI runners and cause instability. Consider implementing environment-specific limits:
- maxConcurrency: os.cpus().length, - maxWorkers: "100%", + maxConcurrency: process.env.CI ? Math.min(os.cpus().length, 4) : os.cpus().length, + maxWorkers: process.env.CI ? "50%" : "100%",
74-74: Consider increasing testTimeout for complex testsThe 5-second timeout might be too restrictive for tests involving complex operations or network calls. Consider increasing it to at least 10 seconds:
- testTimeout: 5000, + testTimeout: 10000,package.json (1)
14-16: The scripts section looks good, but consider enhancing the test script.While the basic
"test": "jest"configuration works, consider adding additional test scripts for common scenarios:"scripts": { "lint:fix": "npm-run-all --parallel --continue-on-error \"lint:check:* -- --fix\"", "start": "webpack serve --open --config webpack.dev.js", - "test": "jest" + "test": "jest", + "test:watch": "jest --watch", + "test:coverage": "jest --coverage" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/test.yaml(1 hunks)jest.config.ts(1 hunks)package.json(6 hunks)test/utils/math.test.ts(1 hunks)tsconfig.json(1 hunks)
🔇 Additional comments (9)
.github/workflows/test.yaml (1)
16-20: LGTM! Concurrency settings are well configured.
The configuration properly handles parallel runs and resource efficiency.
test/utils/math.test.ts (1)
1-5: LGTM! Clean and well-structured imports.
The import statement follows TypeScript best practices and maintains good readability.
jest.config.ts (2)
88-88: LGTM! Clean and standard export syntax.
26-28: Create a timeline for removing coverage overrides
The TODO comment indicates these overrides are temporary. Consider creating specific tickets to track the implementation of test coverage for each directory to ensure this technical debt is addressed.
Let's verify which directories currently have tests:
package.json (3)
46-47: Dependencies are well-structured and appropriate.
The added test dependencies are correctly configured:
- Jest and its TypeScript integration packages
- Appropriate type definitions
- Consistent version range usage
Also applies to: 58-58, 69-71
92-102: ESLint test configuration is appropriate.
The test-specific ESLint overrides are well-configured:
- Correctly enables Jest environment
- Reasonably disables no-magic-numbers for test files
Line range hint 107-120: Import restrictions are properly formatted.
The no-restricted-imports rule maintains the same functionality with improved readability.
tsconfig.json (2)
3-5: LGTM! The include paths are properly configured for Jest integration.
The additions of "jest.config.ts" and "test" directory to the include paths are appropriate for enabling TypeScript compilation and type checking of the Jest configuration and test files.
3-5: Verify test files comply with strict TypeScript rules.
The project uses strict TypeScript configuration. Let's ensure all test files follow these rules.
✅ Verification successful
Test files comply with strict TypeScript rules
The test files are properly typed and follow strict TypeScript rules as evidenced by:
-
The tsconfig.json has strict mode enabled with all strict type-checking options:
"strict": true"noImplicitAny": true"strictNullChecks": true- And all other strict flags are enabled
-
Examination of test/utils/math.test.ts shows:
- No usage of
anytype - No type assertions
- No type-check suppressions
- Properly typed function parameters and return values
- Clean TypeScript code following strict rules
- No usage of
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any TypeScript errors in test files
# Expected: No type errors in test files
# Search for any @ts-ignore or @ts-nocheck comments in test files
echo "Checking for type-check suppressions in test files..."
rg -g 'test/**/*.ts' '@ts-(ignore|nocheck)'
# Search for any 'any' type usage in test files
echo "Checking for 'any' type usage in test files..."
rg -g 'test/**/*.ts' ': any'
Length of output: 302
Script:
#!/bin/bash
# Let's try a different approach to verify TypeScript strictness in test files
# First, let's check if test files exist and their locations
echo "Locating TypeScript test files..."
fd -e ts -p "test/"
# Check for common TypeScript non-strict patterns
echo -e "\nChecking for non-strict patterns in test files..."
rg -g 'test/**/*.ts' -i "(\?:|undefined|null).*=.*;" --multiline
# Look for type assertions
echo -e "\nChecking for type assertions..."
rg -g 'test/**/*.ts' "as \w+"
# Check for explicit 'any' type in function parameters
echo -e "\nChecking function parameters for 'any' type..."
ast-grep --pattern '(parameter) $_: any'
Length of output: 571
Script:
#!/bin/bash
# Let's examine the actual content of the test file to verify TypeScript strictness
# Display the content of the test file
echo "Content of test/utils/math.test.ts:"
cat test/utils/math.test.ts
# Check tsconfig.json for strict settings
echo -e "\nChecking TypeScript configuration:"
cat tsconfig.json
Length of output: 13431
junhaoliao
changed the title
jest for test automation.jest config with unit tests for utils/math.ts; Add gh-actions workflow test for running the tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/test.yaml (1)
1-10: Consider expanding PR event types and improving schedule visibility.The workflow triggers are well-configured, but consider these enhancements:
- Add more PR event types like "ready_for_review" to ensure comprehensive coverage
- Move the schedule explanation to a more visible block comment
name: "test" on: pull_request: - types: ["opened", "reopened", "synchronize"] + types: ["opened", "reopened", "synchronize", "ready_for_review"] push: + # Schedule configuration + # ------------------- + # Runs daily at 00:15 UTC + # Delayed by 15 minutes to avoid GitHub Actions peak load times schedule: - # Run at midnight UTC every day with 15 minutes delay added to avoid high load periods - cron: "15 0 * * *" workflow_dispatch:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.github/workflows/test.yaml(1 hunks)jest.config.ts(1 hunks)package.json(6 hunks)test/utils/math.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- jest.config.ts
- package.json
- test/utils/math.test.ts
- tsconfig.json
🔇 Additional comments (3)
.github/workflows/test.yaml (3)
12-15: Previous comment about missing permissions still applies.
16-20: Excellent concurrency configuration!
The concurrency settings are well-structured with:
- Appropriate grouping strategy using workflow name and ref
- Efficient handling of in-progress jobs
21-34: Previous comment about job configuration improvements still applies.
# Conflicts: # package-lock.json
|
hey @kirkrodrigues , do you want to check if you agree with the coverage numbers before we merge this PR? |
package.json
Outdated
| @@ -6,13 +6,14 @@ | |||
| "scripts": { | |||
| "analyze:size": "webpack --config webpack.prod.js --profile --json=bundle-stats.json && webpack-bundle-analyzer bundle-stats.json dist/", | |||
| "build": "webpack --config webpack.prod.js", | |||
| "start": "webpack serve --open --config webpack.dev.js", | |||
| "lint": "npm run lint:check", | |||
| "lint:check": "npm-run-all --sequential --continue-on-error lint:check:*", | |||
| "lint:check:css": "stylelint src/**/*.css", | |||
| "lint:check:js": "eslint src webpack.*.js --max-warnings 0", | |||
There was a problem hiding this comment.
We should add the new files to linting, right?
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
jest.config.ts (2)
7-14: Simplify the environment check and logging.The environment check and logging can be simplified for better readability.
Consider this cleaner implementation:
-if ("undefined" !== typeof process.env.GITHUB_ACTIONS) { +if (process.env.GITHUB_ACTIONS) { PRIMARY_REPORTER = [ "github-actions", {silent: false}, ]; } -console.log(`Environment variable "GITHUB_ACTIONS"="${process.env.GITHUB_ACTIONS}": ` + - `primary reporter will be ${JSON.stringify(PRIMARY_REPORTER)}`); +console.log(`Using reporter: ${JSON.stringify(PRIMARY_REPORTER)} (GITHUB_ACTIONS=${process.env.GITHUB_ACTIONS})`);
61-80: Document configuration decisions.The configuration includes several specific values that would benefit from documentation:
- Why 5000ms for testTimeout?
- The implications of using worker threads
- The reason for choosing ESM in the transform configuration
Consider adding JSDoc comments to explain these choices:
+/** + * Performance optimization settings: + * - maxConcurrency: Utilizes all available CPU cores + * - maxWorkers: Allows maximum parallel execution + * - workerThreads: Enables parallel test execution + */ maxConcurrency: os.cpus().length, maxWorkers: "100%", +/** + * Test execution settings: + * - testTimeout: 5000ms to accommodate async operations + * - transform: Uses ESM for modern module support + */ testTimeout: 5000,
jest.config.ts
Outdated
| coverageThreshold: { | ||
| "global": { | ||
| functions: 90, | ||
| lines: 90, | ||
| }, | ||
| // eslint-disable-next-line no-warning-comments | ||
| // TODO: Remove/adjust the overrides below as more test cases are added. | ||
| "src/*.tsx": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/components/": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/contexts/": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/services/": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/typings/": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/utils/": { | ||
| functions: 0, | ||
| lines: 0, | ||
| }, | ||
| "src/utils/math.ts": { | ||
| functions: 100, | ||
| lines: 100, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider stricter coverage requirements.
Based on previous discussions:
- Branch coverage metric is missing from the configuration
- Consider starting with 100% coverage globally and adding exceptions as needed, rather than the current approach
Consider this revised structure:
coverageThreshold: {
"global": {
- functions: 90,
- lines: 90,
+ branches: 100,
+ functions: 100,
+ lines: 100,
},
- // eslint-disable-next-line no-warning-comments
- // TODO: Remove/adjust the overrides below as more test cases are added.
- "src/*.tsx": {
- functions: 0,
- lines: 0,
- },
+ // Add specific exceptions below with clear justification comments
"src/components/": {
+ // TODO: Implement UI component testing (Issue #XXX)
functions: 0,
lines: 0,
- },
- // ... other 0% configurations ...
+ }
},Committable suggestion skipped: line range outside the PR's diff.
…MARY_REPORTER with quotes.
…s from code review Co-authored-by: kirkrodrigues <[email protected]>
package.json
Outdated
| "lint": "npm run lint:check", | ||
| "lint:check": "npm-run-all --sequential --continue-on-error lint:check:*", | ||
| "lint:check:css": "stylelint src/**/*.css", | ||
| "lint:check:js": "eslint src webpack.*.js --max-warnings 0", | ||
| "lint:check:js": "eslint jest.config.ts src webpack.*.js --max-warnings 0", |
There was a problem hiding this comment.
Don't we want to lint the test files too?
There was a problem hiding this comment.
Sorry. Addressed in e4f90d4
junhaoliao
changed the title
jest config with unit tests for utils/math.ts; Add gh-actions workflow test for running the tests.
kirkrodrigues
changed the title
Description
jestconfig and npm scripttestto run tests. The config:<projectRoot>/test.src/utils/math.ts.testfor running the tests and reporting the results..and addignorePatternsin ESLint config so that the test files and the jest configuration file are also linted.Validation performed
The
testworkflow was automatically triggered on PR pushes. Below results matched expectation:https://github.com/y-scope/yscope-log-viewer/actions/runs/11759387295
src/utils/math.ts:https://github.com/y-scope/yscope-log-viewer/actions/runs/11759373956
Also, for ESLint, made lint violation in math.test.ts and
src/App.ts, rannpm run lint:check:jsand observed the violations getting reported.Summary by CodeRabbit
New Features
package.jsonto include a new test script and additional dependencies.Bug Fixes
clampandgetChunkNumfunctions.Chores