fix: broken e2e tests #1391
fix: broken e2e tests #1391
Conversation
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 focuses on enhancing the reliability of end-to-end tests. It addresses dependency management by enforcing a hoisted linker strategy for 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
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }); | ||
| const nodes = await settlemint.blockchainNode.list(env.SETTLEMINT_APPLICATION!); | ||
| return nodes.find((node) => node.name === blockchainNodeName); | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect Access Token Usage in Tests
Local findBlockchainNodeByName function uses env.SETTLEMINT_ACCESS_TOKEN instead of the E2E-specific process.env.SETTLEMINT_ACCESS_TOKEN_E2E_TESTS that should be used (as defined in the existing utility function at test/utils/test-resources.ts). This will cause the function to use the wrong access token, potentially failing authentication or accessing the wrong environment. The existing utility function should be imported and used instead, as done in other test files like pause-resume-blockchain-node.e2e.test.ts.
|
To view in Slack, search for: 1761641080.660299 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/create-new-standalone-project.e2e.test.ts:127` </location>
<code_context>
await updatePackageJsonToUseLinkedDependencies(dAppDir);
await updatePackageJsonToUseLinkedDependencies(contractsDir);
await updatePackageJsonToUseLinkedDependencies(subgraphDir);
- await $`bun install --no-cache`.cwd(projectDir).env(env);
+ await $`bun install --no-cache --linker=hoisted`.cwd(projectDir).env(env);
});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions to verify successful installation after changing the linker.
Please add assertions to confirm that all dependencies are present and correctly installed after using the hoisted linker.
Suggested implementation:
```typescript
await $`bun install --no-cache --linker=hoisted`.cwd(projectDir).env(env);
// Assert that node_modules exists and key dependencies are installed
const fs = require("fs");
const path = require("path");
const nodeModulesPath = path.join(projectDir, "node_modules");
expect(fs.existsSync(nodeModulesPath)).toBe(true);
// Check for a few expected dependencies (replace with actual dependencies as needed)
const expectedDeps = ["bun", "typescript"];
for (const dep of expectedDeps) {
const depPath = path.join(nodeModulesPath, dep);
expect(fs.existsSync(depPath)).toBe(true);
}
});
```
- You may need to adjust the `expectedDeps` array to match the actual dependencies your project installs.
- If your test environment uses ES modules, replace `require` with `import` statements.
- Ensure that the test runner has access to the `fs` and `path` modules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
github-actions
bot
added
the
status:ready-for-review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix broken end-to-end tests. The changes include adding the --linker=hoisted flag to bun install commands, which is a reasonable fix for dependency issues. Another change involves correctly identifying a blockchain node by its unique name instead of a display name in the restart test. While this logic is correct, it introduces a duplicated helper function in test/restart-resources.e2e.test.ts. I've left comments suggesting how to resolve this duplication to improve code maintainability, along with a note on an unused import.
| async function findBlockchainNodeByName(blockchainNodeName: string) { | ||
| const env: Partial<DotEnv> = await loadEnv(false, false); | ||
| const settlemint = createSettleMintClient({ | ||
| accessToken: env.SETTLEMINT_ACCESS_TOKEN!, | ||
| instance: env.SETTLEMINT_INSTANCE!, | ||
| }); | ||
| const nodes = await settlemint.blockchainNode.list(env.SETTLEMINT_APPLICATION!); | ||
| return nodes.find((node) => node.name === blockchainNodeName); | ||
| } |
There was a problem hiding this comment.
This findBlockchainNodeByName function is a duplicate of an existing utility in test/utils/test-resources.ts. While the implementation is slightly different regarding the access token, duplicating code increases maintenance overhead and can lead to inconsistencies. A better approach would be to refactor the shared utility to be more flexible. For now, please rename this function to avoid name collisions and confusion with the shared utility (e.g., findBlockchainNodeByNameWithDefaultToken). This will make the code's intent clearer.
| import type { DotEnv } from "@settlemint/sdk-utils/validation"; | ||
| import { NODE_NAME_3_WITHOUT_PK } from "./constants/test-resources"; | ||
| import { forceExitAllCommands, runCommand } from "./utils/run-command"; | ||
| import { setupSettleMintClient } from "./utils/test-resources"; |
There was a problem hiding this comment.
The setupSettleMintClient function is imported but never used in this file. Unused imports should be removed to improve code readability and maintainability.
📦 Packages
|
github-actions
bot
added
qa:success
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| async function findBlockchainNodeByName(blockchainNodeName: string) { | ||
| const env: Partial<DotEnv> = await loadEnv(false, false); | ||
| const settlemint = createSettleMintClient({ | ||
| accessToken: env.SETTLEMINT_ACCESS_TOKEN!, | ||
| instance: env.SETTLEMINT_INSTANCE!, | ||
| }); | ||
| const nodes = await settlemint.blockchainNode.list(env.SETTLEMINT_APPLICATION!); | ||
| return nodes.find((node) => node.name === blockchainNodeName); | ||
| } |
There was a problem hiding this comment.
Bug: The findBlockchainNodeByName function uses the wrong environment variable for SettleMint client authentication in E2E tests.
Severity: HIGH | Confidence: 0.95
🔍 Detailed Analysis
The findBlockchainNodeByName function initializes the SettleMint client using env.SETTLEMINT_ACCESS_TOKEN! for authentication. This variable is incorrect for E2E tests, as the test runner populates process.env.SETTLEMINT_ACCESS_TOKEN_E2E_TESTS! with the necessary credentials. The loadEnv() function does not specifically populate SETTLEMINT_ACCESS_TOKEN with E2E test credentials, leading to an authentication failure when the client attempts to list blockchain nodes.
💡 Suggested Fix
Modify the findBlockchainNodeByName function to use process.env.SETTLEMINT_ACCESS_TOKEN_E2E_TESTS! for the accessToken when creating the SettleMint client, or reuse the existing setupSettleMintClient() utility.
🤖 Prompt for AI Agent
Fix this bug. In test/restart-resources.e2e.test.ts at lines 41-49: The
`findBlockchainNodeByName` function initializes the SettleMint client using
`env.SETTLEMINT_ACCESS_TOKEN!` for authentication. This variable is incorrect for E2E
tests, as the test runner populates `process.env.SETTLEMINT_ACCESS_TOKEN_E2E_TESTS!`
with the necessary credentials. The `loadEnv()` function does not specifically populate
`SETTLEMINT_ACCESS_TOKEN` with E2E test credentials, leading to an authentication
failure when the client attempts to list blockchain nodes.
Did we get this right? 👍 / 👎 to inform future reviews.
|
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. 📝 WalkthroughWalkthroughThe pull request modifies three end-to-end test files. In two test files ( Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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.
ℹ️ 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".
| async function findBlockchainNodeByName(blockchainNodeName: string) { | ||
| const env: Partial<DotEnv> = await loadEnv(false, false); | ||
| const settlemint = createSettleMintClient({ | ||
| accessToken: env.SETTLEMINT_ACCESS_TOKEN!, | ||
| instance: env.SETTLEMINT_INSTANCE!, |
There was a problem hiding this comment.
The new helper builds the SDK client with env.SETTLEMINT_ACCESS_TOKEN loaded from .env, even though the test suite relies on SETTLEMINT_ACCESS_TOKEN_E2E_TESTS for authentication (see test/utils/test-resources.ts and other e2e tests). In environments where only the E2E token is provided and no .env file exists with SETTLEMINT_ACCESS_TOKEN, the client will be created with undefined credentials and settlemint.blockchainNode.list will fail with 401, so the test will still break. Reuse setupSettleMintClient() or read process.env.SETTLEMINT_ACCESS_TOKEN_E2E_TESTS to ensure the call is authenticated.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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)
test/create-new-settlemint-project.e2e.test.ts(1 hunks)test/create-new-standalone-project.e2e.test.ts(1 hunks)test/restart-resources.e2e.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/create-new-settlemint-project.e2e.test.tstest/create-new-standalone-project.e2e.test.tstest/restart-resources.e2e.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{ts,tsx}: Write unit tests using Vitest
Mock external API calls in tests
Test error scenarios explicitly
Files:
test/create-new-settlemint-project.e2e.test.tstest/create-new-standalone-project.e2e.test.tstest/restart-resources.e2e.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create end-to-end tests for CLI commands
Files:
test/create-new-settlemint-project.e2e.test.tstest/create-new-standalone-project.e2e.test.tstest/restart-resources.e2e.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/bun.mdc)
Use
Bun.File()for file reading and writing
Files:
test/create-new-settlemint-project.e2e.test.tstest/create-new-standalone-project.e2e.test.tstest/restart-resources.e2e.test.ts
🧬 Code graph analysis (1)
test/restart-resources.e2e.test.ts (3)
test/utils/test-resources.ts (1)
findBlockchainNodeByName(32-37)sdk/utils/src/validation/dot-env.schema.ts (1)
DotEnv(112-112)sdk/js/src/settlemint.ts (1)
createSettleMintClient(275-426)
🪛 Biome (2.1.2)
test/restart-resources.e2e.test.ts
[error] 8-8: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Unsafe fix: Remove the unused imports.
(lint/correctness/noUnusedImports)
⏰ 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). (5)
- GitHub Check: QA
- GitHub Check: cubic · AI code reviewer
- GitHub Check: E2E
- GitHub Check: Sourcery review
- GitHub Check: QA
🔇 Additional comments (3)
test/create-new-settlemint-project.e2e.test.ts (1)
80-80: LGTM: Hoisted linker flag aligns with local SDK linking.The
--linker=hoistedflag ensures a flat dependency structure, which is appropriate when linking local SDK packages.test/create-new-standalone-project.e2e.test.ts (1)
127-127: LGTM: Consistent hoisted linker flag.This matches the change in
test/create-new-settlemint-project.e2e.test.tsand ensures consistent behavior across test files.test/restart-resources.e2e.test.ts (1)
20-28: Dynamic node lookup improves test reliability.The change from a hardcoded constant to dynamically looking up the node by name and using its
uniqueNamemakes the test more robust. The error handling for missing nodes is appropriate.
| import { createSettleMintClient } from "@settlemint/sdk-js"; | ||
| import { loadEnv } from "@settlemint/sdk-utils/environment"; | ||
| import { fetchWithRetry } from "@settlemint/sdk-utils/http"; | ||
| import type { DotEnv } from "@settlemint/sdk-utils/validation"; | ||
| import { NODE_NAME_3_WITHOUT_PK } from "./constants/test-resources"; | ||
| import { forceExitAllCommands, runCommand } from "./utils/run-command"; | ||
| import { setupSettleMintClient } from "./utils/test-resources"; |
There was a problem hiding this comment.
Remove unused import and use existing helper instead.
The setupSettleMintClient import on line 8 is unused. Additionally, test/utils/test-resources.ts already exports a findBlockchainNodeByName function (lines 31-36) that uses setupSettleMintClient. Import and use that existing helper instead of redefining it locally.
Apply this diff to remove the unused import:
-import { setupSettleMintClient } from "./utils/test-resources";
+import { findBlockchainNodeByName } from "./utils/test-resources";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createSettleMintClient } from "@settlemint/sdk-js"; | |
| import { loadEnv } from "@settlemint/sdk-utils/environment"; | |
| import { fetchWithRetry } from "@settlemint/sdk-utils/http"; | |
| import type { DotEnv } from "@settlemint/sdk-utils/validation"; | |
| import { NODE_NAME_3_WITHOUT_PK } from "./constants/test-resources"; | |
| import { forceExitAllCommands, runCommand } from "./utils/run-command"; | |
| import { setupSettleMintClient } from "./utils/test-resources"; | |
| import { createSettleMintClient } from "@settlemint/sdk-js"; | |
| import { loadEnv } from "@settlemint/sdk-utils/environment"; | |
| import { fetchWithRetry } from "@settlemint/sdk-utils/http"; | |
| import type { DotEnv } from "@settlemint/sdk-utils/validation"; | |
| import { NODE_NAME_3_WITHOUT_PK } from "./constants/test-resources"; | |
| import { forceExitAllCommands, runCommand } from "./utils/run-command"; | |
| import { findBlockchainNodeByName } from "./utils/test-resources"; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 8-8: This import is unused.
Unused imports might be the result of an incomplete refactoring.
Unsafe fix: Remove the unused imports.
(lint/correctness/noUnusedImports)
🤖 Prompt for AI Agents
In test/restart-resources.e2e.test.ts around lines 2 to 8, remove the unused
import setupSettleMintClient and instead import findBlockchainNodeByName from
the existing test/utils/test-resources module; update the test to call
findBlockchainNodeByName where the local helper was being redefined or used so
the code reuses the shared implementation and eliminates the unused import.
| async function findBlockchainNodeByName(blockchainNodeName: string) { | ||
| const env: Partial<DotEnv> = await loadEnv(false, false); | ||
| const settlemint = createSettleMintClient({ | ||
| accessToken: env.SETTLEMINT_ACCESS_TOKEN!, | ||
| instance: env.SETTLEMINT_INSTANCE!, | ||
| }); | ||
| const nodes = await settlemint.blockchainNode.list(env.SETTLEMINT_APPLICATION!); | ||
| return nodes.find((node) => node.name === blockchainNodeName); | ||
| } |
There was a problem hiding this comment.
Remove duplicate helper - reuse existing implementation.
This function duplicates findBlockchainNodeByName already defined in test/utils/test-resources.ts (lines 31-36). The existing implementation uses setupSettleMintClient(), which is the appropriate test utility wrapper.
Remove this local function definition and import the existing helper as suggested in the earlier comment (lines 2-8).
-
-async function findBlockchainNodeByName(blockchainNodeName: string) {
- const env: Partial<DotEnv> = await loadEnv(false, false);
- const settlemint = createSettleMintClient({
- accessToken: env.SETTLEMINT_ACCESS_TOKEN!,
- instance: env.SETTLEMINT_INSTANCE!,
- });
- const nodes = await settlemint.blockchainNode.list(env.SETTLEMINT_APPLICATION!);
- return nodes.find((node) => node.name === blockchainNodeName);
-}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/restart-resources.e2e.test.ts around lines 41 to 49, remove the local
duplicate findBlockchainNodeByName function and instead import and reuse the
existing helper from test/utils/test-resources.ts (the existing implementation
at lines 31-36 uses setupSettleMintClient()); delete the local function, add the
appropriate import at the top (per the earlier comment around lines 2-8), and
update any calls to use the imported helper so the test uses the shared utility.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="test/restart-resources.e2e.test.ts">
<violation number="1" location="test/restart-resources.e2e.test.ts:44">
Use the E2E-specific access token environment variable for authentication in tests to avoid targeting the wrong environment. Prefer reusing the shared client setup helper or switch this line to the E2E token.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| async function findBlockchainNodeByName(blockchainNodeName: string) { | ||
| const env: Partial<DotEnv> = await loadEnv(false, false); | ||
| const settlemint = createSettleMintClient({ | ||
| accessToken: env.SETTLEMINT_ACCESS_TOKEN!, |
There was a problem hiding this comment.
Use the E2E-specific access token environment variable for authentication in tests to avoid targeting the wrong environment. Prefer reusing the shared client setup helper or switch this line to the E2E token.
Prompt for AI agents
Address the following comment on test/restart-resources.e2e.test.ts at line 44:
<comment>Use the E2E-specific access token environment variable for authentication in tests to avoid targeting the wrong environment. Prefer reusing the shared client setup helper or switch this line to the E2E token.</comment>
<file context>
@@ -30,3 +37,13 @@ describe("Restart platform resources using the SDK", () => {
+async function findBlockchainNodeByName(blockchainNodeName: string) {
+ const env: Partial<DotEnv> = await loadEnv(false, false);
+ const settlemint = createSettleMintClient({
+ accessToken: env.SETTLEMINT_ACCESS_TOKEN!,
+ instance: env.SETTLEMINT_INSTANCE!,
+ });
</file context>
Summary by Sourcery
Use dynamic node lookup and hoisted bun linker in e2e tests to fix broken tests
Bug Fixes:
--linker=hoistedfor correct dependency installation