-
Notifications
You must be signed in to change notification settings - Fork 181
chore: standardize test tooling and type defs #904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: standardize test tooling and type defs #904
Conversation
WalkthroughCleaned CI/review config in .coderabbit.yaml and bumped several dev/test dependency versions across multiple package.json files; no source code or public API changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.coderabbit.yaml(0 hunks)app/package.json(1 hunks)circuits/package.json(1 hunks)common/package.json(1 hunks)contracts/package.json(2 hunks)sdk/qrcode/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .coderabbit.yaml
🔇 Additional comments (4)
sdk/qrcode/package.json (1)
103-103: Mocha bump looks fine; no functional impact on this workspaceMocha is dev-only here and "test" is a no-op, so this bump is low risk and aligns with the PR goal of standardizing versions.
app/package.json (1)
162-162: Run workspace type-check — Corepack/Yarn mismatch blocked automated verificationI couldn't run the requested type-check in the sandbox: the repo's packageManager is [email protected] but the environment has Yarn 1.22.22. Please run the workspace type-check locally/CI and report results.
Commands you can run:
# recommended (enable Corepack) corepack enable corepack prepare [email protected] --activate yarn workspace @selfxyz/mobile-app types # one-liner corepack enable && yarn workspace @selfxyz/mobile-app types # alternative one-off without enabling Corepack npx -y [email protected] workspace @selfxyz/mobile-app typesQuick context & what to check:
- File: app/package.json (around line 162)
"@types/react": "^18.3.4",- This is a types-only bump (no runtime impact). Verify CI/type-check for the mobile workspace with strict mode enabled — React 18.3's types tightened some JSX-related definitions and may introduce type errors in components. If errors appear, treat them as medium-priority fixes (adjust component prop typings or JSX intrinsic declarations).
circuits/package.json (1)
101-103: Action required — verify mocha/chai bump locally (Corepack/Yarn‑4 required) before mergingQuick summary: I inspected the repo — circuits/package.json has chai "^4.4.1", mocha "^10.7.3" and ts-mocha "^10.0.0" (test-base uses ts-mocha -n import=tsx). yarn.lock shows mocha resolved to 10.8.2 and ts-mocha 10.1.0. I could not run the workspace tests in the sandbox because the repo declares packageManager "[email protected]" but the environment had Yarn v1 (Corepack must be enabled).
Please verify the upgrade by running the tests locally and checking the areas below:
Files/locations to check
- circuits/package.json — devDependencies: "chai": "^4.4.1", "mocha": "^10.7.3", "ts-mocha": "^10.0.0"; scripts.test-base uses ts-mocha (tsx import loader).
- circuits/scripts/test-base & circuits/tests/**/*.test.ts — test runner invocation and test code (describe/it + chai assertions).
- yarn.lock — mocha resolved to 10.8.2 and ts-mocha to 10.1.0 (confirm lockfile after installs).
Reproduction steps (run locally)
- Enable Corepack and confirm Yarn 4:
- corepack enable
- yarn -v (should be 4.x)
- Run the circuits tests (force reporter and a longer timeout):
- yarn workspace @selfxyz/circuits test-base 'tests/**/*.test.ts' --exit -- --reporter spec --timeout 10000
- If failures persist, isolate mocha as the cause by temporarily pinning an earlier mocha:
- yarn workspace @selfxyz/circuits add -D [email protected]
- yarn workspace @selfxyz/circuits test
- Then restore: yarn workspace @selfxyz/circuits add -D mocha@^10.7.3
What to look for
- Timeouts or different test ordering/parallel behaviour (increase timeout or add --timeout in test-base if flaky).
- Changed assertion formatting from chai/mocha that hides root cause.
- Any ts-mocha + Node 22 ESM loader warnings/errors (try running with NODE_OPTIONS='--loader ts-node/esm' if you see ESM loader issues).
If you can run the above and paste failing test output (or confirm pinning mocha makes failures disappear), I’ll update the review with a concrete fix. I couldn’t complete the verification here because Corepack/Yarn mismatch prevented running tests in the sandbox.
contracts/package.json (1)
108-108: @types/jest bump is safe and scoped to typing onlyNo behavioral risk for contracts; proceed.
Summary
Testing
yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account for network: mainnet)yarn typesyarn workspace @selfxyz/circuits test*(fails: 37 failing)yarn workspace @selfxyz/common testyarn workspace @selfxyz/qrcode testyarn workspace @selfxyz/contracts test(fails: Invalid account for network: mainnet)yarn workspace @selfxyz/mobile-app testhttps://chatgpt.com/codex/tasks/task_b_689e8bd032d8832dad7620b95d8bed9c
Summary by CodeRabbit