-
Notifications
You must be signed in to change notification settings - Fork 2
fix: e2e tests, dev build and concurrent e2e tests #1345
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
Conversation
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.
Sorry @janb87, your pull request is larger than the review limit of 150000 diff characters
Summary of ChangesHello @janb87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request by janb87 addresses test infrastructure improvements by configuring concurrent test execution and extending Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
To view in Slack, search for: 1759925529.574969 |
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.
Code Review
This pull request introduces a couple of important changes to the testing setup, namely enabling concurrent test execution and adjusting timeouts. While these are good steps, there are a couple of critical issues in the implementation that need to be addressed. I've left detailed comments on them.
📦 Packages
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 👍.
|
Related Documentation Checked 6 published document(s). No updates required. You have 10 draft document(s). Publish docs to keep them always up-to-date |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough
Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bunfig.toml(1 hunks)sdk/js/schema.graphql(19 hunks)test/scripts/setup-platform-resources.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Do not use default exports in TypeScript (except when a framework requires it)
Useimport typefor type-only imports in TypeScript
Prefer interfaces over type aliases for object shapes
Never useany; useunknownor proper types instead
Use discriminated unions for error handling
File names should be kebab-case
Use camelCase for variable and function names
Use PascalCase for types, interfaces, and classes
Use UPPER_SNAKE_CASE for constants
Prefer nullish coalescing (??) over logical OR (||)
Use early returns to reduce nesting
Extract complex logic into well-named functions
Keep functions small and focused
Use structured logging with proper context
**/*.{ts,tsx}: Inside generic functions, using any for concise type narrowing is acceptable; outside of generics, use any extremely sparingly
Avoid default exports unless explicitly required by the framework (e.g., Next.js pages)
Use discriminated unions to model variant data and avoid 'bag of optionals'
Handle discriminated unions with switch statements over the discriminant
Do not introduce new enums; prefer as const objects for enum-like behavior; retain existing enums
Use top-level import type when importing types instead of inline import { type ... }
Prefer interface extends over intersection types (&) for modeling inheritance; use & only when extends is not possible
Use JSDoc comments to annotate functions and types when behavior isn’t obvious; keep comments concise
Use JSDoc inline @link to reference related functions and types within the same file
File names should be kebab-case (e.g., my-component.ts)
Use camelCase for variables and function names
Use UpperCamelCase (PascalCase) for classes, types, and interfaces
Use ALL_CAPS for constants (and enum values where enums already exist)
Prefix generic type parameters with T (e.g., TKey, TValue)
Be mindful of noUncheckedIndexedAccess: treat indexed object/array access as possibly undefined
Use optional properties sparingly; prefer required prop...
Files:
test/scripts/setup-platform-resources.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create end-to-end tests for CLI commands
Files:
test/scripts/setup-platform-resources.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/bun.mdc)
Use
Bun.File()for file reading and writing
Files:
test/scripts/setup-platform-resources.ts
🧠 Learnings (2)
📚 Learning: 2025-10-06T23:18:37.400Z
Learnt from: CR
PR: settlemint/sdk#0
File: .cursor/rules/bun.mdc:0-0
Timestamp: 2025-10-06T23:18:37.400Z
Learning: Applies to {package.json,**/jest.config.@(js|ts|cjs|mjs)} : Use `bun run test` (Bun test runner) instead of Jest
Applied to files:
test/scripts/setup-platform-resources.ts
📚 Learning: 2025-10-06T23:18:37.400Z
Learnt from: CR
PR: settlemint/sdk#0
File: .cursor/rules/bun.mdc:0-0
Timestamp: 2025-10-06T23:18:37.400Z
Learning: Applies to {package.json,**/vitest.config.@(js|ts|cjs|mjs)} : Use `bun run test` (Bun test runner) instead of Vitest
Applied to files:
test/scripts/setup-platform-resources.ts
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: QA
- GitHub Check: QA
🔇 Additional comments (2)
bunfig.toml (1)
9-9: Fix TOML syntax forconcurrentTestGlob.
concurrentTestGlob="./test/**/*.test.ts"violates the TOML key/value separator rule (requires whitespace after=), so bun won’t be able to parse this section. Please add the missing spaces.-concurrentTestGlob="./test/**/*.test.ts" +concurrentTestGlob = "./test/**/*.test.ts"⛔ Skipped due to learnings
Learnt from: CR PR: settlemint/sdk#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-06T23:17:27.585Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Test error scenariosLearnt from: CR PR: settlemint/sdk#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-06T23:17:29.417Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Test error scenarios explicitlyLearnt from: CR PR: settlemint/sdk#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-06T23:17:22.726Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Test error scenarios explicitlyLearnt from: CR PR: settlemint/sdk#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-06T23:17:27.585Z Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use test fixtures for consistencyLearnt from: CR PR: settlemint/sdk#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-06T23:17:22.726Z Learning: Applies to test/**/*.ts : Write end-to-end tests for CLI commandssdk/js/schema.graphql (1)
8299-8301: Double-check resolver coverage forisPreRelease.
isPreReleaseis declared asBoolean!, so everyKitresolver must now return a non-null value. If the backing data or serializers have not been updated yet, any request for this field will fail at runtime. Please confirm the upstream resolvers populate it (and consider making it nullable until all data sources are ready).
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.
2 issues found across 4 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="test/scripts/setup-platform-resources.ts">
<violation number="1" location="test/scripts/setup-platform-resources.ts:48">
Timeout value computes to 10 minutes but the trailing comment says 1 hour; align code and comment to avoid confusion.</violation>
<violation number="2" location="test/scripts/setup-platform-resources.ts:101">
Don't force-exit the test process in afterAll; this will always fail the suite. Remove the process.exit(1) and let the runner finish normally, or only exit on explicit error paths.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Documentation Updates Checked 6 published document(s). No updates required. You have 10 draft document(s). Publish docs to keep them always up-to-date |
Summary by cubic
Prevented beforeAll timeouts and enabled concurrent e2e test runs to speed up execution. Updated the GraphQL schema with new fields and deprecations used by the tests.
Bug Fixes
New Features
Note
Updates dev scripts to watch ./src, switches the e2e restart test to target a blockchain node with new expectations, and increases setup timeout; removes a large generated GraphQL types file.
blockchain-nodeusingNODE_NAME_3_WITHOUT_PK; update expected output strings.test/scripts/setup-platform-resources.tsviasetDefaultTimeout(1h).devscript across multiple packages (sdk/mcp,sdk/minio,sdk/next,sdk/portal,sdk/thegraph,sdk/utils,sdk/viem) totsdown --watch ./src.sdk/test/test-app/portal-env.d.ts.Written by Cursor Bugbot for commit 7f6135b. This will update automatically on new commits. Configure here.