Conversation
- Set up vitest as test framework - Extract filterDiagnostics() from App.tsx into rosbagUtils.ts for testability - Add 40 unit tests covering filterMessages and filterDiagnostics - Add docs/testing.md as the test specification document Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add .github/workflows/test.yml that runs vitest on push/PR and publishes test results as PR comments via publish-unit-test-result-action. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Test Results40 tests 40 ✅ 0s ⏱️ Results for commit a05fe4a. ♻️ This comment has been updated with latest results. |
Add paths-ignore to deploy.yml so that changes to test files, docs, and test workflow do not trigger unnecessary deploys. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated unit-test setup for the filtering utilities used by the ROSbag Analyzer UI, extracting diagnostics filtering logic into a testable utility and adding a documented test specification.
Changes:
- Added
filterDiagnostics()tosrc/rosbagUtils.tsand refactoredApp.tsxto use it. - Introduced Vitest (
npm test,npm run test:watch) and added a comprehensive unit test suite forfilterMessages/filterDiagnostics. - Added a testing specification document and CI workflow coverage for running tests.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/rosbagUtils.ts |
Extracts diagnostics filtering into a reusable/exported helper for testability. |
src/rosbagUtils.test.ts |
Adds Vitest unit tests covering many filter combinations and edge cases. |
src/App.tsx |
Switches diagnostics filtering to the new utility and adds a footer link to source. |
package.json |
Adds Vitest scripts and dependencies for running unit tests. |
package-lock.json |
Locks the dependency graph for the new test tooling. |
docs/testing.md |
Adds a written test specification to keep behavior expectations explicit. |
.github/workflows/test.yml |
Adds CI job to run tests and publish JUnit results. |
.github/workflows/deploy.yml |
Avoids triggering deploys for test/spec-only changes. |
Comments suppressed due to low confidence (3)
package.json:39
vitest@^4.0.18(and its dependency chain) requires Node >=20 per the generatedpackage-lock.json, but the repo README currently states Node 18+ is sufficient. This will break installs/tests for contributors on Node 18. Consider either pinning Vitest to a Node-18-compatible major version, or updating the documented Node requirement and adding apackage.jsonengines.nodefield to make the requirement explicit.
"tslib": "^2.8.1",
"typescript": "^5.2.2",
"vite": "^5.0.8",
"vitest": "^4.0.18"
.github/workflows/test.yml:36
- The workflow runs Vitest via
npx vitest ...even thoughpackage.jsondefinesnpm test. Using the npm script (e.g.,npm test -- --reporter=junit ...) avoids duplication and keeps CI aligned with the locally documented command if the script ever changes.
- run: npx vitest run --reporter=junit --outputFile=junit.xml
docs/testing.md:13
- The markdown tables use an extra leading
|(e.g., starting rows with|| ...), which creates an unintended empty first column and renders oddly on GitHub. Remove the extra leading pipe so the tables render correctly.
| Function | File | Description |
|---|---|---|
| `filterMessages` | `src/rosbagUtils.ts` | Filters rosout (`/rosout`, `/rosout_agg`) messages |
| `filterDiagnostics` | `src/rosbagUtils.ts` | Filters diagnostics (`/diagnostics_agg`) entries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/App.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
npm test/npm run test:watchscriptsfilterDiagnostics()from inline logic inApp.tsxintorosbagUtils.tsfor testabilityfilterMessagesandfilterDiagnostics(severity/level, node/name, keywords, regex, time range, OR/AND modes, combined filters, edge cases)docs/testing.mdas the test specification document with numbered test case IDs (FM-xx / FD-xx) to serve as the source of truth for future test additionsTest plan
npm test— all 40 tests passnpm run build— build succeeds with no regressions🤖 Generated with Claude Code