Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 972366571e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
🟡 Changes recommended
The updated packaging/build workflow relies on Lara v4/Vitest artifacts that are not present in the repo’s workspace-local lara-framework/Lara-JS (v3.5.1), making the documented workspace setup and prepack validation fail until dependencies/docs are aligned.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR migrates the Clava-JS test/lint toolchain from Jest/ESLint to Vitest/Oxlint, while also improving the Gradle-to-Node integration by automatically synchronizing the ClavaWeaver installDist output into Clava-JS/java-binaries for packaging and testing.
Changes:
- Replace Jest-based testing with Vitest (new Vitest config, updated tests, new
ClavaLegacyTesterfor Vitest). - Replace ESLint/Prettier scripts with Oxlint/Oxfmt, and update lint suppression directives accordingly.
- Add a Gradle
Synctask to refreshClava-JS/java-binariesafterClavaWeaver:installDist, and update documentation + package metadata for the new workflow.
File summaries
| File | Description |
|---|---|
| ClavaWeaver/build.gradle | Adds a Sync task to mirror installDist output into Clava-JS/java-binaries. |
| Clava-JS/vitest/ClavaLegacyTester.ts | New Vitest-based legacy tester wrapper for Clava integration tests. |
| Clava-JS/vitest.config.ts | Introduces Vitest configuration via Lara’s Vitest config factory. |
| Clava-JS/tsconfig.json | Switches TS global test types from Jest to Vitest. |
| Clava-JS/README.md | Updates build/test instructions to the new workflow (currently needs correction). |
| Clava-JS/package.json | Bumps major version, migrates scripts/deps to Vitest + Oxlint, adds prepack validation. |
| Clava-JS/oxlint.config.ts | Adds Oxlint configuration extending Lara’s base config. |
| Clava-JS/jest/ClavaLegacyTester.ts | Removes Jest-specific legacy tester. |
| Clava-JS/jest.config.ts | Removes Jest config. |
| Clava-JS/eslint.config.ts | Removes ESLint flat config in favor of Oxlint. |
| Clava-JS/CxxTestSources/test1/main.cpp | Removes old test source fixture. |
| Clava-JS/CxxTestSources/test1/lib.h | Removes old test source fixture header. |
| Clava-JS/CxxTestSources/test1/lib.cpp | Removes old test source fixture implementation. |
| Clava-JS/code/jest.config.ts | Removes Jest config for code tests. |
| Clava-JS/code/ClangPlugin/ClangPlugin.test.ts | Updates mocking API from Jest to Vitest (jest → vi). |
| Clava-JS/clava.json | Removes legacy watch config file. |
| Clava-JS/api/Query.test.ts | Updates helpers import from Lara Jest helpers to Lara Vitest helpers. |
| Clava-JS/api/LegacyIntegrationTests - Issues.test.ts | Switches to Vitest tester + updates lint suppression directive. |
| Clava-JS/api/LegacyIntegrationTests - CXX.test.ts | Switches to Vitest tester and adds per-test cleanup for shared state. |
| Clava-JS/api/LegacyIntegrationTests - C.test.ts | Switches to Vitest tester, adds cleanup, and adjusts timeouts. |
| Clava-JS/api/jest.config.ts | Removes Jest config for API tests. |
| Clava-JS/api/Issues.test.ts | Updates helpers import from Lara Jest helpers to Lara Vitest helpers. |
| Clava-JS/api/clava/ClavaJoinPoints.test.ts | Updates helpers import from Lara Jest helpers to Lara Vitest helpers. |
| Clava-JS/api/clava/ClavaJavaTypes.ts | Updates lint suppression directives for Oxlint rule names. |
| .vscode/extensions.json | Recommends Oxlint + Vitest Explorer extensions instead of Jest/Prettier/ESLint. |
Review details
- Files reviewed: 25/29 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Introduce a root Clava Gradle aggregate for Java and Clava-JS build/test tasks, validate the Clava-JS java-binaries symlink against ClavaWeaver installDist, and wire Clava-JS Jest runs to JaCoCo so Node-hosted JVM coverage is merged with Java test coverage.
Adopt the Lara-JS shared lint and formatter configuration, replace Jest test setup with Vitest, and run TypeScript tests through Node natively.
Keep TypeScript explicitly declared for Clava-JS type checking.
Allow the known long-running InlineNasLu and Inliner tests up to ten seconds without changing the suite-wide timeout.
Give the long-running Inliner integration test a fifteen-second timeout while retaining InlineNasLu at ten seconds.
Adopt the shared Oxlint configuration and Oxfmt scripts, retaining lint exclusions with Oxlint directives.
Run API and code tests in one Vitest configuration backed by Lara’s shared Weaver lifecycle helper, while preserving package test filters.
Materialize ClavaWeaver's distribution for every installDist, make aggregate JS builds depend on the synchronized output, and validate npm package contents through Lara's shared helper.
The documented combined Gradle build (clavaJsBuild, clavaMergedJacocoReport) was removed from the repository, leaving README instructions that could not be executed. Replace them with the actual workflow: ClavaWeaver installDist now synchronizes java-binaries directly, followed by the npm workspace builds. Drop the merged JaCoCo report section.
Without the files field, npm falls back to .gitignore, which ignores java-binaries, so the published package shipped without the weaver JARs required by code/WeaverConfiguration.ts. Restore the allowlist, updated for the vitest/oxlint migration.
|
No description provided.