test: add comprehensive Cucumber BDD test suite for Superposition API#914
Open
test: add comprehensive Cucumber BDD test suite for Superposition API#914
Conversation
Contributor
WalkthroughThis PR introduces a comprehensive Cucumber-based BDD test suite for Superposition. It includes feature files for 13 domains (config, organization, workspace, dimension, etc.), step definitions for both API and UI testing, test infrastructure (worlds and hooks), and Playwright configuration for automated UI testing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
knutties
changed the title
Add a comprehensive Cucumber test suite using Gherkin feature files that mirror all existing Bun-based integration tests. The Gherkin feature files are designed to be reusable for both API and Web UI testing - only the step definitions need to change. Covers all 12 API domains: - Organisation, Workspace, Default Config, Config Retrieval - Dimension, Context, Experiment, Experiment Groups - Functions, Variables, Secrets, Type Templates Includes both happy-path and error scenarios, with Scenario Outlines for data-driven tests (e.g. invalid name patterns). https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
Add a complete set of Playwright-based step definitions that reuse the same Gherkin feature files as the API tests. The cucumber.js config now supports two profiles: - api: drives the SDK directly (step_definitions/ + support/) - ui: drives a Playwright browser (step_definitions_ui/ + support_ui/) Usage: npm run test:api # run API tests (default) npm run test:ui # run UI tests headless npm run test:ui:headed # run UI tests with visible browser The PlaywrightWorld class provides navigation helpers, drawer/modal interaction, Monaco editor filling, toast detection, and screenshot capture on failure. Step definitions cover all 12 domains matching the Leptos frontend routes and component patterns. https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
- Switch from ts-node/require to tsx for ESM compatibility - Use cucumber-js --import instead of --require - Use default function export in cucumber.js for profiles - Remove duplicate "at most N item" step definitions - Add all missing UI step definitions for config, type_template, and experiment_group features Both api and ui profiles now pass dry-run cleanly: 126 scenarios, 542 steps, 0 ambiguous, 0 undefined. https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
- Fix dimension position values exceeding workspace count (start at 1) - Fix SDK input format for context commands (require `request` wrapper) - Fix UpdateOverridesExperiment to pass all variants in variant_list - Fix error matching to extract raw server body from SDK $response - Update expected error strings from "Parse error" to "Json deserialize error" - Fix org/workspace lookup with pagination (count: 200) and smart selection - Disable change_reason_validation on test workspace - Remove underscore from uniqueName() for workspace name compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use position 1 for dimensions to avoid exceeding dimension count limit - Add missing description field to bulk PUT context operations - Fix experiment conclude test to use 50% ramp (server max is 50%) - Update function test expectation: compute functions with string returns succeed (validated at runtime) - Update workspace test: listing with invalid org returns empty, not error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rage The UI step definitions were written speculatively without knowledge of the actual Leptos/DaisyUI frontend. Rewrote all 14 step definition files plus world/hooks to use the SDK client (same as API tests), enabling all 126 scenarios to pass for both API and UI profiles. - Add SuperpositionClient to PlaywrightWorld for SDK-based operations - Add SDK org/workspace setup and resource cleanup to UI hooks - Rewrite all step_definitions_ui/*.ts to use SDK calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rewrite UI step definitions to use a hybrid approach: Playwright browser interactions for happy-path create/list operations (workspaces, variables, secrets) and SDK calls for error cases where UI toast detection is unreliable. This ensures actual UI testing while maintaining reliable error assertions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ctions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… interactions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… interactions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Navigate to the overrides page, find the context card by its ID attribute on the ConditionComponent ol element, open the dropdown menu, click "Delete Overrides", wait for the ChangeLogPopup confirmation modal, click "Yes, Delete", then verify deletion by waiting for the card to disappear from the DOM. Pre-checks context existence via SDK to avoid Playwright timeouts on missing entities. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Navigate to overrides page, find context card by ID attribute selector, click dropdown menu → Delete Overrides → confirm modal, then verify the context element is removed from the page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…raction Pre-check group existence via SDK GetExperimentGroupCommand, navigate to the experiment-groups listing page and verify the row is visible in the table (using the group UUID as the tr element id), then attempt the actual delete via SDK. The UI listing page currently has no delete button for experiment groups, so the SDK is used for the delete action while Playwright validates the group's presence in the UI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… steps Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… and config steps Convert SDK-only get/update steps to hybrid Playwright+SDK: - Experiment get and update overrides navigate to detail page - Variable get navigates to detail page - Secret get navigates to detail page - Organisation update navigates to org list page - Config get navigates to default-config page - Experiment group steps navigate to list/detail pages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Also fix unused rowCount variable in list step. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
knutties
force-pushed
the
claude/cucumber-gherkin-tests-CScOB
branch
from
4ad40c0 to
4611e2d
Compare
- Add npm ci step for cucumber test dependencies before running tests - Remove UI tests from CI (requires Playwright + Chromium not available) - Fix SDK API mismatches after rebase: - Remove nonexistent 'description' from UpdateWorkspaceCommand - Fix CreateDimensionCommand schema type to Record<string, any> - Remove nonexistent 'status' from ListWorkspaceCommand https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
Install Chromium via `npx playwright install --with-deps chromium` before running UI tests so headless browser is available in CI. https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
- Add playwright-driver.browsers to flake.nix devShell packages - Set PLAYWRIGHT_BROWSERS_PATH in shellHook for nix-provided browsers - Pin playwright to 1.52.0 to match nixpkgs playwright-driver version - Make makefile browser install conditional: skip when PLAYWRIGHT_BROWSERS_PATH is set (nix shell), install via npx otherwise (CI/non-nix) https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
There was a problem hiding this comment.
Pull request overview
Adds a Cucumber/Gherkin BDD test suite under tests/cucumber to exercise the Superposition API (and a parallel UI profile backed by Playwright), and wires it into the repo’s make test flow.
Changes:
- Introduce Cucumber test runner config + TypeScript setup for step definitions and shared World state.
- Add API step definitions + Gherkin feature coverage across org/workspace/config/dimensions/contexts/experiments/experiment-groups/functions/variables/secrets/type-templates/config resolution.
- Add UI profile step definitions + Playwright world/hooks, and run both API + UI suites from
make test.
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| makefile | Runs cucumber API + UI suites as part of make test. |
| tests/cucumber/.gitignore | Ignores build artifacts and generated reports for the cucumber package. |
| tests/cucumber/cucumber.js | Defines api and ui Cucumber profiles and HTML report outputs. |
| tests/cucumber/package.json | Adds the cucumber test package (TypeScript + cucumber + Playwright + SDK). |
| tests/cucumber/tsconfig.json | TypeScript compiler configuration for cucumber step/support code. |
| tests/cucumber/support/world.ts | Defines SuperpositionWorld (API profile shared state + SDK client). |
| tests/cucumber/support/hooks.ts | API profile lifecycle hooks for shared org/workspace setup and per-scenario cleanup. |
| tests/cucumber/support_ui/world.ts | Defines PlaywrightWorld (UI profile shared state + Playwright objects + SDK client). |
| tests/cucumber/support_ui/hooks.ts | UI profile hooks: browser lifecycle, per-scenario context/page, screenshots, SDK cleanup. |
| tests/cucumber/step_definitions/common_steps.ts | Generic API assertions for success/failure and response shape checks. |
| tests/cucumber/step_definitions/organisation_steps.ts | API org create/get/list/update steps. |
| tests/cucumber/step_definitions/workspace_steps.ts | API workspace create/list/update/filter steps. |
| tests/cucumber/step_definitions/dimension_steps.ts | API dimension CRUD steps. |
| tests/cucumber/step_definitions/default_config_steps.ts | API default-config create/update validation scenarios and function validation hooks. |
| tests/cucumber/step_definitions/context_steps.ts | API context create/get/list/update/move/delete/bulk/weight-recompute steps. |
| tests/cucumber/step_definitions/experiment_steps.ts | API experiment lifecycle steps (create/list/get/ramp/override/conclude/discard). |
| tests/cucumber/step_definitions/experiment_group_steps.ts | API experiment-group lifecycle and membership/traffic/sorting steps. |
| tests/cucumber/step_definitions/function_steps.ts | API function CRUD + publish steps. |
| tests/cucumber/step_definitions/variable_steps.ts | API variable CRUD + function integration steps. |
| tests/cucumber/step_definitions/secret_steps.ts | API secret CRUD + compute-function integration steps. |
| tests/cucumber/step_definitions/type_template_steps.ts | API type-template CRUD steps. |
| tests/cucumber/step_definitions/config_steps.ts | API config retrieval + version pin/unpin + list versions steps. |
| tests/cucumber/step_definitions/resolve_config_steps.ts | API identifier-bucketing config resolution steps. |
| tests/cucumber/step_definitions_ui/common_steps.ts | Generic UI-profile assertions reusing World lastResponse/lastError. |
| tests/cucumber/step_definitions_ui/organisation_steps.ts | UI-profile org steps (UI navigation checks + SDK for operations). |
| tests/cucumber/step_definitions_ui/workspace_steps.ts | UI-profile workspace steps (UI for happy-path, SDK for error cases). |
| tests/cucumber/step_definitions_ui/dimension_steps.ts | UI-profile dimension steps (SDK create + UI verification / edit/delete flows). |
| tests/cucumber/step_definitions_ui/context_steps.ts | UI-profile context steps (UI presence checks + SDK operations; UI delete flow). |
| tests/cucumber/step_definitions_ui/experiment_steps.ts | UI-profile experiment steps (UI flows where feasible + SDK fallbacks). |
| tests/cucumber/step_definitions_ui/function_steps.ts | UI-profile function steps (SDK create/update/publish + UI verification). |
| tests/cucumber/step_definitions_ui/variable_steps.ts | UI-profile variable steps (UI create/edit/delete + SDK assertions/fallbacks). |
| tests/cucumber/step_definitions_ui/secret_steps.ts | UI-profile secret steps (UI create/edit/delete + SDK assertions/fallbacks). |
| tests/cucumber/step_definitions_ui/config_steps.ts | UI-profile config retrieval/pin/unpin/version list steps (UI navigation + SDK ops). |
| tests/cucumber/step_definitions_ui/resolve_config_steps.ts | UI-profile identifier-bucketing config resolution steps (SDK). |
| tests/cucumber/features/organisation.feature | Gherkin coverage for organisation management. |
| tests/cucumber/features/workspace.feature | Gherkin coverage for workspace management. |
| tests/cucumber/features/dimension.feature | Gherkin coverage for dimensions. |
| tests/cucumber/features/default_config.feature | Gherkin coverage for default configs (schema + functions). |
| tests/cucumber/features/context.feature | Gherkin coverage for contexts and overrides. |
| tests/cucumber/features/experiment.feature | Gherkin coverage for experiments lifecycle. |
| tests/cucumber/features/experiment_group.feature | Gherkin coverage for experiment groups. |
| tests/cucumber/features/function.feature | Gherkin coverage for functions and publishing. |
| tests/cucumber/features/variable.feature | Gherkin coverage for variables and function integration. |
| tests/cucumber/features/secret.feature | Gherkin coverage for secrets and compute function integration. |
| tests/cucumber/features/type_template.feature | Gherkin coverage for type templates. |
| tests/cucumber/features/config.feature | Gherkin coverage for config retrieval and version pinning. |
| tests/cucumber/features/resolve_config.feature | Gherkin coverage for identifier bucketing config resolution. |
Files not reviewed (1)
- tests/cucumber/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
+125
| When( | ||
| "I list workspaces filtered by status {string}", | ||
| async function (this: SuperpositionWorld, status: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new ListWorkspaceCommand({ | ||
| count: 5, | ||
| page: 1, | ||
| org_id: this.orgId, | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } |
There was a problem hiding this comment.
status parameter is ignored here. This step calls ListWorkspaceCommand without status, so the scenario "List workspaces filtered by ENABLED status" isn't actually exercising filtering and the assertion can fail if DISABLED workspaces exist. Pass status: status as WorkspaceStatus into ListWorkspaceCommand (as done in the UI step definitions).
Comment on lines
+167
to
+172
| if (scenario.result?.status === Status.FAILED) { | ||
| const name = scenario.pickle.name.replace(/\s+/g, "-").toLowerCase(); | ||
| await this.page.screenshot({ | ||
| path: `reports/screenshots/${name}.png`, | ||
| fullPage: true, | ||
| }); |
There was a problem hiding this comment.
The screenshot path reports/screenshots/... will fail with ENOENT if the directory doesn't exist, which can cause the After hook to throw and skip cleanup (leaving test resources behind). Ensure reports/screenshots is created before taking screenshots (e.g., mkdir in BeforeAll/Before).
Comment on lines
+12
to
+20
| api: { | ||
| import: ["step_definitions/**/*.ts", "support/**/*.ts"], | ||
| format: ["progress", "html:reports/api-report.html"], | ||
| formatOptions: { snippetInterface: "async-await" }, | ||
| }, | ||
| ui: { | ||
| import: ["step_definitions_ui/**/*.ts", "support_ui/**/*.ts"], | ||
| format: ["progress", "html:reports/ui-report.html"], | ||
| formatOptions: { snippetInterface: "async-await" }, |
There was a problem hiding this comment.
Both profiles write HTML reports under reports/..., but the config doesn't ensure the reports/ directory exists. Depending on the formatter implementation, this can fail the run before any scenarios execute. Create reports/ (and reports/screenshots/ for UI) as part of setup (e.g., in a BeforeAll hook or a pretest script).
Comment on lines
+2
to
+16
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "bundler", | ||
| "esModuleInterop": true, | ||
| "strict": true, | ||
| "outDir": "./dist", | ||
| "rootDir": ".", | ||
| "declaration": true, | ||
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "resolveJsonModule": true | ||
| }, | ||
| "include": ["step_definitions/**/*.ts", "support/**/*.ts"] | ||
| } |
There was a problem hiding this comment.
This tsconfig will fail tsc typechecking/emit with the current imports: the step/support files use explicit .ts import specifiers (e.g. ../support/world.ts), but allowImportingTsExtensions is not enabled. Either remove the .ts extensions from imports, or set allowImportingTsExtensions: true and switch to noEmit / emitDeclarationOnly (TS disallows emitting runnable JS with .ts specifiers). Also consider including the UI globs (step_definitions_ui/**, support_ui/**) so those files are typechecked too.
Comment on lines
+21
to
+26
| // Track group-specific experiment IDs | ||
| let validExpId: string; | ||
| let validExp2Id: string; | ||
| let inProgressExpId: string; | ||
| let conflictingContextExpId: string; | ||
|
|
There was a problem hiding this comment.
These module-level mutable variables (validExpId, validExp2Id, etc.) introduce hidden cross-scenario state. If scenarios ever run in parallel (or if step ordering/background changes), this can lead to flaky behavior and makes isolation harder to reason about. Prefer storing these IDs on the SuperpositionWorld instance (per-scenario) or deriving them from this.lastResponse in each scenario.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
tests/cucumber/tsconfig.json-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorMissing include patterns for UI test directories.
The
includearray only coversstep_definitions/**/*.tsandsupport/**/*.ts, but thecucumber.jsconfiguration shows that the UI profile imports fromstep_definitions_ui/**/*.tsandsupport_ui/**/*.ts. This means TypeScript type-checking won't apply to UI test files.Proposed fix to include UI directories
- "include": ["step_definitions/**/*.ts", "support/**/*.ts"] + "include": [ + "step_definitions/**/*.ts", + "support/**/*.ts", + "step_definitions_ui/**/*.ts", + "support_ui/**/*.ts" + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/tsconfig.json` at line 15, The tsconfig.json "include" array is missing UI test patterns, so TypeScript won't type-check UI step definitions and support files; update the include array (the "include" key in tsconfig.json) to also include "step_definitions_ui/**/*.ts" and "support_ui/**/*.ts" alongside the existing "step_definitions/**/*.ts" and "support/**/*.ts" so the UI profile files referenced by cucumber.js are type-checked.tests/cucumber/step_definitions/dimension_steps.ts-132-151 (1)
132-151:⚠️ Potential issue | 🟡 MinorSame unused parameter issue in update and delete steps.
Both
"I update dimension {string}"and"I delete dimension {string}"capture thenameparameter but ignore it, usingthis.dimensionNameinstead. Consider applying the same fix as suggested for the get step.Also applies to: 153-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/dimension_steps.ts` around lines 132 - 151, The step definitions for the "I update dimension {string} description to {string}" (handler using UpdateDimensionCommand) and the similar delete step (lines ~153-174) ignore the captured name parameter and use this.dimensionName instead; fix by using the provided name when building the command (or by assigning this.dimensionName = name at the start of the step) so UpdateDimensionCommand and DeleteDimensionCommand use the passed-in name rather than the existing this.dimensionName; update both the update handler and the delete handler accordingly.tests/cucumber/cucumber.js-17-21 (1)
17-21:⚠️ Potential issue | 🟡 MinorUI profile directories not included in tsconfig.json.
The
uiprofile imports fromstep_definitions_ui/**/*.tsandsupport_ui/**/*.ts, but per the relevant snippet,tsconfig.jsononly includesstep_definitions/**/*.tsandsupport/**/*.ts. Whiletsxexecutes TypeScript directly (bypassingtsc), this means:
- No compile-time type checking for UI step definitions
- IDE support may be degraded in those directories
Consider updating
tsconfig.jsonto include the UI directories.Suggested tsconfig.json update
- "include": ["step_definitions/**/*.ts", "support/**/*.ts"] + "include": ["step_definitions/**/*.ts", "support/**/*.ts", "step_definitions_ui/**/*.ts", "support_ui/**/*.ts"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/cucumber.js` around lines 17 - 21, The UI profile in cucumber.js imports step_definitions_ui/**/*.ts and support_ui/**/*.ts but tsconfig.json only includes step_definitions/**/*.ts and support/**/*.ts, so update tsconfig.json's "include" array to add the UI folders (e.g., add "step_definitions_ui/**/*.ts" and "support_ui/**/*.ts") so those files are picked up by TypeScript/IDE; locate the tsconfig.json include section and append the two UI glob patterns to ensure compile-time checking and editor support for the UI step definitions referenced by the ui profile in cucumber.js.tests/cucumber/step_definitions/dimension_steps.ts-97-114 (1)
97-114:⚠️ Potential issue | 🟡 MinorStep parameter
nameis ignored;this.dimensionNameis used instead.The step definition captures
{string}asnamebut ignores it, always usingthis.dimensionNamefrom the world state. This could cause unexpected behavior if a scenario tries to get a dimension by a different name. Either remove the parameter from the Gherkin step pattern or use it.Option 1: Use the captured parameter
When( "I get dimension {string}", async function (this: SuperpositionWorld, name: string) { + const targetName = this.uniqueName(name); try { this.lastResponse = await this.client.send( new GetDimensionCommand({ workspace_id: this.workspaceId, org_id: this.orgId, - dimension: this.dimensionName, + dimension: targetName, }) );Option 2: Remove unused parameter from step pattern
Update the Gherkin in
dimension.featureto:When I get the dimensionAnd the step to:
When("I get the dimension", async function (this: SuperpositionWorld) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/dimension_steps.ts` around lines 97 - 114, The step handler currently ignores the captured parameter name and always uses this.dimensionName; update the When handler to use the incoming name parameter for the GetDimensionCommand (replace this.dimensionName with name) so the request reflects the Gherkin string, and ensure any tests that rely on this.dimensionName are adjusted if needed; alternatively, if the step should not accept a parameter, remove the "{string}" from the step pattern and the name parameter from the function signature to keep the Gherkin and step definition consistent.tests/cucumber/package.json-38-40 (1)
38-40:⚠️ Potential issue | 🟡 MinorDowngrade
@types/nodeto match the project's Node.js version.The main CI test job runs Node.js 18.19.0, but
@types/nodeis pinned to v22. This mismatch should be resolved by changing@types/nodeto^18.0.0to align with the actual runtime version used in the test environment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/package.json` around lines 38 - 40, Update the devDependency version for `@types/node` in package.json to match the project's Node.js runtime: change the entry for "@types/node" from "^22.0.0" to "^18.0.0" so the type definitions align with Node.js 18.19.0 used by CI (edit the devDependencies block where "@types/node" is declared).tests/cucumber/step_definitions/context_steps.ts-252-277 (1)
252-277:⚠️ Potential issue | 🟡 MinorTrack created context IDs from bulk operation response for cleanup.
The
BulkOperationCommandcreates contexts but doesn't add their IDs tothis.createdContextIds. This leaves contexts orphaned after test runs since the cleanup hooks iterate only over tracked IDs. Extract IDs fromthis.lastResponse.outputarray and track PUT, REPLACE, and MOVE operation results.🧹 Corrected fix to track bulk-created contexts
When( "I perform a bulk operation to create contexts for {string} values {string}", async function (this: SuperpositionWorld, dimName: string, values: string) { const valueList = values.split(",").map((v) => v.trim()); try { this.lastResponse = await this.client.send( new BulkOperationCommand({ workspace_id: this.workspaceId, org_id: this.orgId, operations: valueList.map((val) => ({ PUT: { context: { [dimName]: val }, override: { "ctx-config-key": `${val}-bulk` }, description: `Bulk context for ${val}`, change_reason: "Cucumber bulk test", }, })), }) ); + // Track created context IDs from bulk operation response + if (this.lastResponse?.output) { + for (const action of this.lastResponse.output) { + if (action.PUT?.id) this.createdContextIds.push(action.PUT.id); + if (action.REPLACE?.id) this.createdContextIds.push(action.REPLACE.id); + if (action.MOVE?.id) this.createdContextIds.push(action.MOVE.id); + } + } this.lastError = undefined; } catch (e: any) { this.lastError = e; this.lastResponse = undefined; } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/context_steps.ts` around lines 252 - 277, The bulk-op step handler does not record IDs of created/modified contexts so cleanup misses them; after the successful call in the When handler (the async function that sends new BulkOperationCommand) inspect this.lastResponse.output (or the response body) and extract the context IDs from entries for PUT, REPLACE and MOVE operations, then push those IDs into this.createdContextIds so cleanup hooks can remove them; update the code immediately after setting this.lastResponse (before clearing this.lastError) to iterate the output array, collect IDs from relevant result objects, and add them to this.createdContextIds.tests/cucumber/support_ui/hooks.ts-165-178 (1)
165-178:⚠️ Potential issue | 🟡 MinorGuard against uninitialized
pagebefore taking screenshot.If the
Beforehook fails beforethis.pageis created (e.g.,browser.newContext()throws), callingthis.page.screenshot()will throw an unhandled error sincepagehas a definite assignment assertion (!) but may be undefined at runtime.🛡️ Proposed fix to add null check
After(async function (this: PlaywrightWorld, scenario) { // Take screenshot on failure for debugging - if (scenario.result?.status === Status.FAILED) { + if (scenario.result?.status === Status.FAILED && this.page) { const name = scenario.pickle.name.replace(/\s+/g, "-").toLowerCase(); await this.page.screenshot({ path: `reports/screenshots/${name}.png`, fullPage: true, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/support_ui/hooks.ts` around lines 165 - 178, The After hook currently assumes this.page is always initialized and calls this.page.screenshot, which can throw if the Before hook failed; update the After hook in the After(async function (this: PlaywrightWorld, scenario) { ... }) to guard against an uninitialized page by checking that this.page is defined (and optionally this.page.isClosed() is false) before calling this.page.screenshot, and keep the existing cleanup of this.context (await this.context.close()) but also guard that this.context is defined before closing.tests/cucumber/step_definitions/experiment_group_steps.ts-530-552 (1)
530-552:⚠️ Potential issue | 🟡 Minor
sortOnparameter mapping always returnsCREATED_ATregardless of input.The ternary expression
sortOn === "created_at" ? ExperimentGroupSortOn.CREATED_AT : ExperimentGroupSortOn.CREATED_ATalways evaluates toCREATED_AT, making the parameter effectively ignored.🐛 Proposed fix
If only
created_atis supported, simplify or document it:When( "I list experiment groups sorted by {string} in {string} order", async function (this: SuperpositionWorld, sortOn: string, sortBy: string) { try { this.lastResponse = await this.client.send( new ListExperimentGroupsCommand({ workspace_id: this.workspaceId, org_id: this.orgId, - sort_on: - sortOn === "created_at" - ? ExperimentGroupSortOn.CREATED_AT - : ExperimentGroupSortOn.CREATED_AT, + sort_on: ExperimentGroupSortOn.CREATED_AT, // Only created_at is currently supported sort_by: sortBy === "DESC" ? SortBy.DESC : SortBy.ASC, count: 5, }) );Or if multiple sort options should be supported, implement proper mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/experiment_group_steps.ts` around lines 530 - 552, The step definition's ternary in the When handler always maps sortOn to ExperimentGroupSortOn.CREATED_AT, so update the mapping used when constructing the ListExperimentGroupsCommand (in this async function in experiment_group_steps.ts) to return the correct ExperimentGroupSortOn value for each supported sort key (e.g., map "created_at" -> ExperimentGroupSortOn.CREATED_AT, "name" -> ExperimentGroupSortOn.NAME, etc.), or if only created_at is supported, replace the conditional with the single constant ExperimentGroupSortOn.CREATED_AT and remove the dead branch; ensure the symbol ExperimentGroupSortOn used in the command payload reflects the intended sort field.tests/cucumber/step_definitions_ui/context_steps.ts-348-373 (1)
348-373:⚠️ Potential issue | 🟡 MinorBulk operation created contexts are not tracked for cleanup.
The bulk operation creates multiple contexts but doesn't capture and track the returned context IDs in
createdContextIds. This will leave orphaned test data if the bulk operation succeeds but subsequent steps fail.🛠️ Proposed fix to track bulk-created context IDs
When( "I perform a bulk operation to create contexts for {string} values {string}", async function (this: PlaywrightWorld, dimName: string, values: string) { const valueList = values.split(",").map((v) => v.trim()); try { this.lastResponse = await this.client.send( new BulkOperationCommand({ workspace_id: this.workspaceId, org_id: this.orgId, operations: valueList.map((val) => ({ PUT: { context: { [dimName]: val }, override: { "ctx-config-key": `${val}-bulk` }, description: `Bulk context for ${val}`, change_reason: "Cucumber bulk test", }, })), }) ); + // Track created context IDs for cleanup + if (Array.isArray(this.lastResponse)) { + for (const result of this.lastResponse) { + if (result?.context_id) { + this.createdContextIds.push(result.context_id); + } + } + } this.lastError = undefined; } catch (e: any) { this.lastError = e; this.lastResponse = undefined; } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/context_steps.ts` around lines 348 - 373, The bulk-operation step doesn't record IDs of contexts it creates; after a successful BulkOperationCommand call in the When handler (the async function for "I perform a bulk operation..."), extract created context IDs from this.lastResponse (e.g., from a response field like results/operations/items that contains created context IDs) and append them to the test's createdContextIds array so they can be cleaned up later; add this extraction and push logic immediately after setting this.lastResponse (and before clearing this.lastError) in the same try block, and guard against missing response shapes (check for undefined) when reading IDs.tests/cucumber/step_definitions_ui/workspace_steps.ts-48-71 (1)
48-71:⚠️ Potential issue | 🟡 MinorEmpty name handling could produce unexpected unique names.
When
nameis empty (for error case testing),this.uniqueName("")will produce a string like"lxyz123"(just the testRunId), which may not trigger the intended validation error since it becomes a non-empty alphanumeric string. TheisValidNameregex check happens on the originalname, but the SDK receivesuniqueName.🛠️ Proposed fix to preserve empty names for error cases
When( "I create a workspace with name {string} and admin email {string}", async function (this: PlaywrightWorld, name: string, email: string) { - const uniqueName = name ? this.uniqueName(name) : name; + const uniqueName = name ? this.uniqueName(name) : ""; // Use SDK for error cases (empty name, special chars, invalid email) const isValidName = /^[a-zA-Z0-9]+$/.test(name); - if (!isValidName) { + if (!isValidName || name === "") { try { this.lastResponse = await this.client.send( new CreateWorkspaceCommand({ org_id: this.orgId, workspace_admin_email: email, - workspace_name: uniqueName, + workspace_name: name, // Use original name to test validation workspace_status: WorkspaceStatus.ENABLED, }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/workspace_steps.ts` around lines 48 - 71, The test currently calls this.uniqueName(name) even when name is empty, causing empty-name error tests to become valid names; change the logic so uniqueName is only generated when name is non-empty (e.g., compute uniqueName = name ? this.uniqueName(name) : name) or explicitly pass the original name to CreateWorkspaceCommand in the error branch; update the error-handling branch that constructs new CreateWorkspaceCommand (references: uniqueName, name, isValidName, CreateWorkspaceCommand, workspace_name) to ensure an intentionally empty name is sent when name === "" so validation fails as expected.tests/cucumber/step_definitions_ui/experiment_group_steps.ts-597-615 (1)
597-615:⚠️ Potential issue | 🟡 Minor
sortOnis ignored here.Both branches resolve to
ExperimentGroupSortOn.CREATED_AT, so any scenario that asks for a different sort key still exercisescreated_atand can pass for the wrong reason. Map the incoming value to the correct enum, or reject unsupported keys explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts` around lines 597 - 615, The step handler for "I list experiment groups sorted by ..." ignores the sortOn argument because both branches return ExperimentGroupSortOn.CREATED_AT; update the mapping before calling ListExperimentGroupsCommand to translate the incoming sortOn string into the correct ExperimentGroupSortOn enum (e.g., map "created_at" -> ExperimentGroupSortOn.CREATED_AT, other supported keys -> their enums) and for unsupported values either throw an error or explicitly fail the test; ensure the mapped value is passed as the sort_on field to ListExperimentGroupsCommand so the sort key is actually applied.tests/cucumber/step_definitions_ui/experiment_group_steps.ts-771-780 (1)
771-780:⚠️ Potential issue | 🟡 MinorCheck the whole ordering, not just the first pair.
Comparing only
data[0]anddata[1]misses any inversion later in the page. A partially unsorted result set can still pass.Suggested fix
function (this: PlaywrightWorld) { const data = this.lastResponse?.data; assert.ok(Array.isArray(data), "Response is not a list"); - if (data.length > 1) { - const d1 = new Date(data[0].created_at).getTime(); - const d2 = new Date(data[1].created_at).getTime(); - assert.ok(d1 >= d2, "Results not sorted by created_at DESC"); + for (let i = 1; i < data.length; i++) { + const prev = new Date(data[i - 1].created_at).getTime(); + const curr = new Date(data[i].created_at).getTime(); + assert.ok(prev >= curr, `Results not sorted by created_at DESC at index ${i}`); } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts` around lines 771 - 780, The test step "the response should be sorted by created_at descending" currently only compares data[0] and data[1]; update the assertion to verify the entire array is sorted by looping over this.lastResponse?.data and checking each adjacent pair of created_at timestamps (e.g., for i from 0 to data.length-2 ensure new Date(data[i].created_at).getTime() >= new Date(data[i+1].created_at).getTime()); keep the initial Array.isArray check and use the same assertion message "Results not sorted by created_at DESC" when any inversion is found so failures remain clear (refer to the step function using PlaywrightWorld and this.lastResponse).tests/cucumber/step_definitions_ui/organisation_steps.ts-114-121 (1)
114-121:⚠️ Potential issue | 🟡 MinorUnused
dataarray - dead code.The
dataarray is populated from table rows (lines 114-121) but is never used. Only the SDK response is stored inlastResponse(line 127). Either remove the unused collection or use it for UI-specific assertions.🧹 Option 1: Remove unused code
When("I list all organisations", async function (this: PlaywrightWorld) { try { await this.goToOrganisations(); // Wait for the table to be visible await this.page.locator("table").waitFor({ state: "visible", timeout: 10000 }); - const rowCount = await this.page.locator("table tbody tr").count(); - - // Collect org data from table rows - const data: any[] = []; - for (let i = 0; i < rowCount; i++) { - const row = this.page.locator("table tbody tr").nth(i); - const cellText = await row.textContent(); - if (cellText) { - data.push({ id: cellText.trim() }); - } - } // Also fetch via SDK to get full details for assertions that need them const sdkResponse = await this.client.send( new ListOrganisationCommand({}) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/organisation_steps.ts` around lines 114 - 121, The local array data is populated from the table rows in the loop using rowCount and this.page.locator(...) but never used afterward; either remove the unused data collection entirely or use it for UI assertions—specifically either delete the data variable and the for loop that builds it, or replace the no-op by comparing/assigning it to lastResponse (or using it in an assertion) so the scraped UI rows are actually validated against lastResponse in the surrounding step definition (referencing data, rowCount, this.page.locator, and lastResponse to locate the code to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cucumber/step_definitions_ui/default_config_steps.ts`:
- Around line 46-72: The Create/Publish flow currently skips publishing if
CreateFunctionCommand throws; modify the logic around CreateFunctionCommand and
PublishCommand so PublishCommand is attempted regardless of whether
CreateFunctionCommand succeeded: call this.client.send(new PublishCommand(...))
in a finally block (or after catching create errors) for each fn so shared
helpers like false_validation/true_function/auto_fn get republished; still push
fn.name into this.createdFunctions only when creation succeeds (or track
published names separately) and ensure any errors from PublishCommand are
handled/logged but do not prevent the loop from continuing.
- Around line 249-348: The tests use raw key values which can collide with
existing defaults and cause false positives; update each failing-case step that
calls CreateDefaultConfigCommand (the blocks with "invalid schema type", "empty
schema", "age ... minimum", and "using validation function") to call
this.uniqueName(key) and use that returned uniqueKey when constructing the
command (and where applicable push it into this.createdConfigs) so the test
exercises schema/function validation rather than duplicate-key errors;
alternatively, remove any pre-existing config for the key before calling
this.client.send, but prefer using this.uniqueName(key) for consistency with
other steps.
In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts`:
- Around line 21-25: The module-scoped experiment ID variables (validExpId,
validExp2Id, inProgressExpId, conflictingContextExpId) must be moved onto the
PlaywrightWorld instance so they are reset by the UI Before hook; remove the
top-level declarations and instead add these properties to the PlaywrightWorld
type and reference them via the world (e.g., world.validExpId,
world.validExp2Id, world.inProgressExpId, world.conflictingContextExpId)
throughout experiment_group_steps.ts; update any imports/uses to access the
world object (or this.playwrightWorld) and ensure the Before hook in
tests/cucumber/support_ui/hooks.ts continues to initialize/clear those world
fields.
- Around line 752-768: Update the step "the list should contain the created
group" to assert the created group's presence rather than just non-empty lists:
check that this.lastResponse?.data is an array and that it includes an item
whose id matches this.experimentGroupId (e.g., find a record with id ===
this.experimentGroupId in the array), and in the UI block after calling
this.goToWorkspacePage("experiment-groups") replace the generic row count check
with a targeted locator that looks for a table row containing the experiment
group identifier (use this.page.locator with a text/has-text match for
this.experimentGroupId) and assert that its count is > 0; keep the UI check as
best-effort (inside the existing try/catch).
In `@tests/cucumber/step_definitions_ui/function_steps.ts`:
- Around line 371-390: The step definition for Then("the response should have
function name {string}") ignores the incoming name parameter and asserts
this.lastResponse.function_name against this.functionName; update the assertion
to compare this.lastResponse.function_name === name and also use name when
checking the UI table (replace this.functionName with name) so both the API
check and the UI best-effort check use the provided name; the change should be
made inside the anonymous async function in the step (symbols: parameter name,
this.lastResponse.function_name, this.functionName, goToWorkspacePage,
page.locator("table")).
- Around line 226-248: The step handler for When("I get function {string}") is
ignoring the incoming name parameter and always using this.functionName; update
the implementation in the When(...) async function to use the local name
variable for navigation and the SDK call (replace this.functionName with name in
calls to this.goToDetailPage and in the new GetFunctionCommand payload), and
optionally set this.functionName = name if subsequent steps expect the world
state to reflect the requested function.
In `@tests/cucumber/step_definitions_ui/variable_steps.ts`:
- Around line 64-68: The test emits a raw secret by logging VARS.${varName}
inside the async execute function in variable_steps.ts; replace the
console.log("API Key:", VARS.${varName}) with a redacted or non-secret
diagnostic (e.g., console.log("API Key: [REDACTED]") or log only
existence/boolean like console.log("API Key set:", !!VARS.${varName}")) so the
boolean assertion (return VARS.${varName} === 'secret-api-key-12345') remains
but no secret value is written to stdout or Cucumber reports.
In `@tests/cucumber/step_definitions/experiment_group_steps.ts`:
- Around line 21-28: The module-level experiment ID variables (validExpId,
validExp2Id, inProgressExpId, conflictingContextExpId) should be moved into the
Cucumber world so they don't persist across scenarios: add corresponding fields
to the SuperpositionWorld class/interface, initialize/reset those fields in the
Before hook (so each scenario starts with fresh undefined/null IDs), and update
all references in experiment_group_steps.ts to access them via this.validExpId,
this.validExp2Id, this.inProgressExpId, and this.conflictingContextExpId instead
of the module-scoped names.
In `@tests/cucumber/step_definitions/workspace_steps.ts`:
- Around line 63-87: The When step creates a workspace but never sets
this.workspaceId, so update the handler (the async function in
workspace_steps.ts that calls CreateWorkspaceCommand) to, after a successful
response (this.lastResponse), extract the created workspace's id from the
response object (e.g., the workspace id field on this.lastResponse or its
payload) and assign it to this.workspaceId (similar to how this.workspaceName is
set); keep the existing try/catch behavior and only set workspaceId when the
CreateWorkspaceCommand call succeeds.
- Around line 110-127: The step handler currently declares a status parameter
but calls ListWorkspaceCommand which does not support status filtering
(ListWorkspaceInput only accepts count, page, all, org_id); either remove the
unused status parameter and any corresponding Gherkin scenario that expects
status-filtered results, or add status filtering support by extending the
ListWorkspaceInput and ListWorkspaceCommand to accept a status field and pass it
through in the call (update the step to include that status in the new input);
locate the When step in workspace_steps.ts and the
ListWorkspaceCommand/ListWorkspaceInput types to apply the chosen change.
- Around line 21-44: The Given step that calls CreateWorkspaceCommand sets
this.workspaceName but never assigns this.workspaceId from the response—capture
the created workspace's id from the CreateWorkspaceCommand response and set
this.workspaceId so subsequent steps use the new workspace; also update the "I
list workspaces filtered by status" step to pass the incoming status parameter
into the ListWorkspaceCommand request so the API call actually filters by the
provided WorkspaceStatus. Use the existing CreateWorkspaceCommand and
ListWorkspaceCommand symbols and update assignments to this.workspaceId and the
ListWorkspaceCommand payload respectively.
In `@tests/cucumber/support/hooks.ts`:
- Around line 137-167: The cleanup currently calls DeleteExperimentGroupCommand
before discarding member experiments, causing group deletes to fail and leak;
change After(this: SuperpositionWorld) so you first iterate
this.createdExperimentIds and send DiscardExperimentCommand for each (using
client.send), then if this.experimentGroupId is set attempt
DeleteExperimentGroupCommand afterwards; additionally, if
DeleteExperimentGroupCommand fails due to members, retry the delete once after
discards (or log the error with context) to ensure groups are removed; reference
the symbols After, SuperpositionWorld, this.createdExperimentIds,
this.experimentGroupId, DiscardExperimentCommand, DeleteExperimentGroupCommand,
and client.send when locating code to modify.
---
Minor comments:
In `@tests/cucumber/cucumber.js`:
- Around line 17-21: The UI profile in cucumber.js imports
step_definitions_ui/**/*.ts and support_ui/**/*.ts but tsconfig.json only
includes step_definitions/**/*.ts and support/**/*.ts, so update tsconfig.json's
"include" array to add the UI folders (e.g., add "step_definitions_ui/**/*.ts"
and "support_ui/**/*.ts") so those files are picked up by TypeScript/IDE; locate
the tsconfig.json include section and append the two UI glob patterns to ensure
compile-time checking and editor support for the UI step definitions referenced
by the ui profile in cucumber.js.
In `@tests/cucumber/package.json`:
- Around line 38-40: Update the devDependency version for `@types/node` in
package.json to match the project's Node.js runtime: change the entry for
"@types/node" from "^22.0.0" to "^18.0.0" so the type definitions align with
Node.js 18.19.0 used by CI (edit the devDependencies block where "@types/node"
is declared).
In `@tests/cucumber/step_definitions_ui/context_steps.ts`:
- Around line 348-373: The bulk-operation step doesn't record IDs of contexts it
creates; after a successful BulkOperationCommand call in the When handler (the
async function for "I perform a bulk operation..."), extract created context IDs
from this.lastResponse (e.g., from a response field like
results/operations/items that contains created context IDs) and append them to
the test's createdContextIds array so they can be cleaned up later; add this
extraction and push logic immediately after setting this.lastResponse (and
before clearing this.lastError) in the same try block, and guard against missing
response shapes (check for undefined) when reading IDs.
In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts`:
- Around line 597-615: The step handler for "I list experiment groups sorted by
..." ignores the sortOn argument because both branches return
ExperimentGroupSortOn.CREATED_AT; update the mapping before calling
ListExperimentGroupsCommand to translate the incoming sortOn string into the
correct ExperimentGroupSortOn enum (e.g., map "created_at" ->
ExperimentGroupSortOn.CREATED_AT, other supported keys -> their enums) and for
unsupported values either throw an error or explicitly fail the test; ensure the
mapped value is passed as the sort_on field to ListExperimentGroupsCommand so
the sort key is actually applied.
- Around line 771-780: The test step "the response should be sorted by
created_at descending" currently only compares data[0] and data[1]; update the
assertion to verify the entire array is sorted by looping over
this.lastResponse?.data and checking each adjacent pair of created_at timestamps
(e.g., for i from 0 to data.length-2 ensure new
Date(data[i].created_at).getTime() >= new Date(data[i+1].created_at).getTime());
keep the initial Array.isArray check and use the same assertion message "Results
not sorted by created_at DESC" when any inversion is found so failures remain
clear (refer to the step function using PlaywrightWorld and this.lastResponse).
In `@tests/cucumber/step_definitions_ui/organisation_steps.ts`:
- Around line 114-121: The local array data is populated from the table rows in
the loop using rowCount and this.page.locator(...) but never used afterward;
either remove the unused data collection entirely or use it for UI
assertions—specifically either delete the data variable and the for loop that
builds it, or replace the no-op by comparing/assigning it to lastResponse (or
using it in an assertion) so the scraped UI rows are actually validated against
lastResponse in the surrounding step definition (referencing data, rowCount,
this.page.locator, and lastResponse to locate the code to change).
In `@tests/cucumber/step_definitions_ui/workspace_steps.ts`:
- Around line 48-71: The test currently calls this.uniqueName(name) even when
name is empty, causing empty-name error tests to become valid names; change the
logic so uniqueName is only generated when name is non-empty (e.g., compute
uniqueName = name ? this.uniqueName(name) : name) or explicitly pass the
original name to CreateWorkspaceCommand in the error branch; update the
error-handling branch that constructs new CreateWorkspaceCommand (references:
uniqueName, name, isValidName, CreateWorkspaceCommand, workspace_name) to ensure
an intentionally empty name is sent when name === "" so validation fails as
expected.
In `@tests/cucumber/step_definitions/context_steps.ts`:
- Around line 252-277: The bulk-op step handler does not record IDs of
created/modified contexts so cleanup misses them; after the successful call in
the When handler (the async function that sends new BulkOperationCommand)
inspect this.lastResponse.output (or the response body) and extract the context
IDs from entries for PUT, REPLACE and MOVE operations, then push those IDs into
this.createdContextIds so cleanup hooks can remove them; update the code
immediately after setting this.lastResponse (before clearing this.lastError) to
iterate the output array, collect IDs from relevant result objects, and add them
to this.createdContextIds.
In `@tests/cucumber/step_definitions/dimension_steps.ts`:
- Around line 132-151: The step definitions for the "I update dimension {string}
description to {string}" (handler using UpdateDimensionCommand) and the similar
delete step (lines ~153-174) ignore the captured name parameter and use
this.dimensionName instead; fix by using the provided name when building the
command (or by assigning this.dimensionName = name at the start of the step) so
UpdateDimensionCommand and DeleteDimensionCommand use the passed-in name rather
than the existing this.dimensionName; update both the update handler and the
delete handler accordingly.
- Around line 97-114: The step handler currently ignores the captured parameter
name and always uses this.dimensionName; update the When handler to use the
incoming name parameter for the GetDimensionCommand (replace this.dimensionName
with name) so the request reflects the Gherkin string, and ensure any tests that
rely on this.dimensionName are adjusted if needed; alternatively, if the step
should not accept a parameter, remove the "{string}" from the step pattern and
the name parameter from the function signature to keep the Gherkin and step
definition consistent.
In `@tests/cucumber/step_definitions/experiment_group_steps.ts`:
- Around line 530-552: The step definition's ternary in the When handler always
maps sortOn to ExperimentGroupSortOn.CREATED_AT, so update the mapping used when
constructing the ListExperimentGroupsCommand (in this async function in
experiment_group_steps.ts) to return the correct ExperimentGroupSortOn value for
each supported sort key (e.g., map "created_at" ->
ExperimentGroupSortOn.CREATED_AT, "name" -> ExperimentGroupSortOn.NAME, etc.),
or if only created_at is supported, replace the conditional with the single
constant ExperimentGroupSortOn.CREATED_AT and remove the dead branch; ensure the
symbol ExperimentGroupSortOn used in the command payload reflects the intended
sort field.
In `@tests/cucumber/support_ui/hooks.ts`:
- Around line 165-178: The After hook currently assumes this.page is always
initialized and calls this.page.screenshot, which can throw if the Before hook
failed; update the After hook in the After(async function (this:
PlaywrightWorld, scenario) { ... }) to guard against an uninitialized page by
checking that this.page is defined (and optionally this.page.isClosed() is
false) before calling this.page.screenshot, and keep the existing cleanup of
this.context (await this.context.close()) but also guard that this.context is
defined before closing.
In `@tests/cucumber/tsconfig.json`:
- Line 15: The tsconfig.json "include" array is missing UI test patterns, so
TypeScript won't type-check UI step definitions and support files; update the
include array (the "include" key in tsconfig.json) to also include
"step_definitions_ui/**/*.ts" and "support_ui/**/*.ts" alongside the existing
"step_definitions/**/*.ts" and "support/**/*.ts" so the UI profile files
referenced by cucumber.js are type-checked.
---
Nitpick comments:
In `@tests/cucumber/.gitignore`:
- Line 4: The .gitignore entry "*.lock" will exclude package-lock.json; update
the file to ensure package-lock.json is committed by either removing the
"*.lock" line or replacing it with explicit patterns for other lockfiles (e.g.,
"yarn.lock", "pnpm-lock.yaml") and add an exception line "!package-lock.json" so
package-lock.json remains tracked.
In `@tests/cucumber/features/dimension.feature`:
- Around line 54-58: Add three new error-case scenarios to the same feature file
alongside "Scenario: Fail to create a dimension with invalid schema": 1)
"Scenario: Fail to create a duplicate dimension" that uses the existing step
"When I create a dimension with name \"test-dim\" and schema type
\"<valid-type>\"" twice and then asserts failure on the second attempt; 2)
"Scenario: Fail to create a dimension with invalid name format" that calls "When
I create a dimension with name \"invalid name!\" and schema type
\"<valid-type>\"" and expects failure; and 3) "Scenario: Fail to delete a
non-existent dimension" that uses "When I delete a dimension with name
\"non-existent-dim\"" and asserts the delete operation fails; reuse existing
step definitions so only the scenario titles and input strings change.
In `@tests/cucumber/features/experiment_group.feature`:
- Around line 101-103: The feature uses inconsistent placeholder IDs for a
non-existent experiment group: change the ID string in the Scenario "Fail to
delete a non-existent group" (currently "22") to the same placeholder used
elsewhere ("123") so all scenarios consistently reference the same non-existent
group ID; update the step "When I delete experiment group \"22\"" to use "123"
to match the other test case.
In `@tests/cucumber/features/experiment.feature`:
- Around line 13-19: The scenario currently builds an experiment across multiple
steps ("When I create an experiment with name ...", "And the experiment has a
control variant ...", "And the experiment has an experimental variant ..."),
which is hard to maintain; refactor by replacing these multiple steps with a
single composite step (e.g., "When I create an experiment named \"exp-test\"
with context \"os\" = \"android\" and variants:" ) that accepts a data table for
variants, or add a new step definition function that takes the experiment name,
context and a list of variants, then update the feature to use that new step so
all creation parameters are declared together (look for the step text strings in
the feature and the corresponding step definitions to change).
- Around line 1-69: Add negative test scenarios to the experiment.feature to
cover error cases: create a new Scenario "Create experiment with invalid
context" that attempts to create an experiment using a malformed/unsupported
context value and asserts an error response; add "Create experiment without
required variants" that tries to create an experiment missing control or
experimental variant and asserts validation failure; add "Ramp an already
concluded experiment" which concludes an experiment then attempts to ramp it and
asserts an error; and add "Conclude a non-ramped experiment" which tries to
conclude an experiment that was never ramped and asserts the appropriate
error—use the existing scenario names and step patterns (e.g., Given an
experiment "...", When I ramp..., When I conclude...) so steps reuse the step
definitions.
In `@tests/cucumber/features/function.feature`:
- Around line 60-66: Replace the generic failure assertion in the "Fail to
create a function with invalid code" scenario by asserting a specific error
pattern instead of just "Then the operation should fail"; update the step that
reads When I create a value_validation function named "invalid-func" with code
"invalid code" followed by Then the operation should fail to use a step that
checks the API error message (e.g., "Then the operation should fail with error
matching /syntax error|invalid code/i" or another regex that matches your API's
actual message), so the test asserts the expected error text and becomes more
robust and self-documenting.
In `@tests/cucumber/features/organisation.feature`:
- Around line 46-52: Add a new scenario in the organisation.feature to
explicitly test the boundary for page 0 (e.g., "Scenario: Fail to list
organisations with zero page") by invoking the existing step "When I list
organisations with count 1 and page 0" and asserting the same error matching
"Page should be greater than 0"; this will clarify whether the API treats
pagination as 1-indexed and complements the existing scenarios that test page -1
and negative count.
In `@tests/cucumber/features/workspace.feature`:
- Around line 48-51: The scenario title "List workspaces with invalid
organisation ID returns empty" contradicts the expectation in the steps; update
the Scenario name or the assertion to match intent: either rename the Scenario
to something explicit like "List workspaces for non-existent organisation
returns empty list" to reflect that a successful call returns an empty list, or
change the step "Then the operation should succeed" to an appropriate failure
assertion if you intend the API to error; ensure you update the Scenario header
and/or the step "When I list workspaces for organisation \"non-existent-org\""
and the final assertion "Then the operation should succeed" so they consistently
express the intended behavior.
In `@tests/cucumber/step_definitions_ui/common_steps.ts`:
- Around line 35-55: There are duplicate Cucumber step definitions of Then("the
response should have an {string} property") in
tests/cucumber/step_definitions_ui/common_steps.ts; remove the redundant
definition and keep a single handler that asserts this.lastResponse and
this.lastResponse[prop] (the existing function using PlaywrightWorld).
Alternatively, replace both with one Cucumber expression that accepts the minor
grammatical variants (e.g., optional "a"/"an") so feature files need not change,
but ensure only one Then(...) for the response property check exists and it
performs the same assert.ok checks on this.lastResponse and
this.lastResponse[prop].
In `@tests/cucumber/step_definitions_ui/config_steps.ts`:
- Around line 1-10: Remove the unused import DeleteDefaultConfigCommand from the
top import list in the step_definitions_ui/config_steps.ts file; locate the
import statement that currently lists GetConfigCommand, ListVersionsCommand,
UpdateWorkspaceCommand, CreateDefaultConfigCommand, DeleteDefaultConfigCommand
and delete only DeleteDefaultConfigCommand so the file no longer imports an
unused symbol (hooks handle cleanup).
- Around line 53-76: Replace hardcoded timeout literals used in this file (e.g.,
the 300 passed to this.page.waitForTimeout in the When step inside the async
function(this: PlaywrightWorld)) with named constants (for example
UI_SETTLE_DELAY_MS, UI_PAGE_LOAD_DELAY_MS) declared at the top of the file or in
a shared test config; update every occurrence of this.page.waitForTimeout(300)
and the other magic numbers noted (lines around the other steps) to use those
constants so the timeouts are maintainable and clearly named.
In `@tests/cucumber/step_definitions_ui/dimension_steps.ts`:
- Around line 97-119: The step currently accepts a captured parameter name but
ignores it, using this.dimensionName instead; update the When handler to use the
captured name consistently by replacing usages of this.dimensionName with the
parameter name (e.g., call this.goToDetailPage("dimensions", name), pass
dimension: name into new GetDimensionCommand, and optionally set
this.dimensionName = name if test state should be updated) so the step behaves
according to the feature file input.
In `@tests/cucumber/step_definitions_ui/function_steps.ts`:
- Around line 81-91: Extract the repeated UI verification into a helper on
PlaywrightWorld: add a method named verifyFunctionInTable(functionName: string)
that wraps the try/catch navigation to this.goToWorkspacePage("function"), waits
for load (e.g., this.page.waitForLoadState("networkidle")), reads the table text
via this.page.locator("table").textContent(), and warns if tableText exists but
does not include functionName; then replace each duplicated block in
function_steps.ts (the try/catch that calls goToWorkspacePage, waits, inspects
table text and warns) with a single await this.verifyFunctionInTable(uniqueName)
call to centralize behavior.
- Around line 43-45: The empty catch block that swallows all errors around the
function-creation call (the try surrounding the create function call in
tests/cucumber/step_definitions_ui/function_steps.ts) should be made specific:
inspect the caught error (e.g., error.message, error.code, or instanceof) and
only suppress it when it indicates the function "already exists" (match the
exact message or error code your API returns); for any other error rethrow or
fail the test so network/auth/transient errors are not hidden. Ensure the change
occurs in the catch that currently contains the comment "// Already exists" so
that only the expected duplicate-create condition is ignored and all other
errors propagate.
- Line 84: Replace the multiple hardcoded this.page.waitForTimeout(300/500)
calls in function_steps.ts with explicit waits: search for all occurrences of
this.page.waitForTimeout and swap each one for an appropriate explicit waiter
such as await this.page.waitForLoadState("networkidle") or await
this.page.locator("<selector>").waitFor({ state: "visible", timeout: 10000 })
using a selector that matches the element the step depends on (e.g., table,
modal, button); update the step helper functions that call
this.page.waitForTimeout to choose the correct explicit waiter per step so tests
wait for load/network/element visibility instead of fixed sleeps.
In `@tests/cucumber/step_definitions_ui/organisation_steps.ts`:
- Around line 159-161: Replace the hardcoded sleep after goToOrganisations() by
waiting for a specific page condition instead of page.waitForTimeout(300);
update the call site in the organisation_steps logic (after
this.goToOrganisations()) to use an explicit wait such as
this.page.waitForLoadState('networkidle') or this.page.waitForSelector(...)
targeting a stable DOM element for the organisations view (e.g., the
organisations list/table or a header) so the test waits deterministically for
the UI to be ready.
- Around line 191-229: The step definitions "the response should have name
{string}" and "the list should contain the created organisation" currently
return early when the UI contains the expected value, which can hide mismatches
between the UI and API; modify both step implementations (the Then handlers) to
always assert against this.lastResponse when present (use
assert.ok(this.lastResponse, ...) and assert.strictEqual(this.lastResponse.name,
...) / check this.lastResponse.data for createdOrgId), and keep the UI checks as
optional additional assertions (e.g., check tableText via
this.page.locator("table").textContent() and assert that it includes the value)
instead of using return to skip API validation; update references to
PlaywrightWorld, this.createdOrgId and this.orgName accordingly so tests
validate both SDK/API and UI consistency.
In `@tests/cucumber/step_definitions_ui/resolve_config_steps.ts`:
- Around line 1-14: Remove the unused imports DiscardExperimentCommand,
DeleteDefaultConfigCommand, and DeleteDimensionCommand from the import list at
the top of resolve_config_steps.ts; locate the import block that brings in
GetResolvedConfigWithIdentifierCommand, CreateDimensionCommand,
CreateDefaultConfigCommand, CreateExperimentCommand, RampExperimentCommand,
DiscardExperimentCommand, DeleteDefaultConfigCommand, DeleteDimensionCommand,
VariantType and delete the three unused symbols so only referenced commands
remain imported.
- Around line 16-103: The Given step for bucketing setup is duplicated between
resolve_config_steps.ts variants (the Given function using PlaywrightWorld here
and the one using SuperpositionWorld elsewhere); extract the shared logic into a
single helper (e.g., setupBucketingTest) that accepts the world-like object
(with client, workspaceId, orgId, uniqueName, createdDimensions, createdConfigs,
createdExperimentIds, configKey, experimentId) and move the
CreateDimensionCommand/CreateDefaultConfigCommand/CreateExperimentCommand/RampExperimentCommand
sequence into that helper, then replace the Given implementations to call
setupBucketingTest(this) so both PlaywrightWorld and SuperpositionWorld reuse
the same code.
In `@tests/cucumber/step_definitions_ui/secret_steps.ts`:
- Around line 289-343: Two identical Cucumber step definitions ("I test the
compute function" and "I test the compute function again") duplicate logic;
refactor by keeping a single When handler that performs the TestCommand send
(using PlaywrightWorld, TestCommand, this.client.send, and setting
this.lastResponse/this.lastError) and replace the other step with a call to that
handler or with a new small helper function (e.g., testCompute()) invoked by
both step definitions so the TestCommand construction and try/catch live in one
location.
In `@tests/cucumber/step_definitions/common_steps.ts`:
- Around line 35-55: Two identical Cucumber step registrations using Then(...)
differ only by the article ("an" vs "a") and should be consolidated; replace the
two separate step definitions with a single step registration for the
response-property check by using a single pattern that accepts both articles
(e.g., a regex or Cucumber expression with an optional "n") and keep the
existing assertion logic (referencing this.lastResponse and the same property
checks) so you only maintain one implementation instead of the duplicate
Then(...) handlers.
In `@tests/cucumber/step_definitions/config_steps.ts`:
- Around line 1-10: Remove the unused DeleteDefaultConfigCommand import from the
import list at the top of the file (where GetConfigCommand, ListVersionsCommand,
UpdateWorkspaceCommand, CreateDefaultConfigCommand, DeleteDefaultConfigCommand
are imported) so the file only imports commands that are actually used; adjust
the remaining import formatting/commas accordingly.
In `@tests/cucumber/step_definitions/default_config_steps.ts`:
- Around line 182-226: The two When step handlers that call
CreateDefaultConfigCommand (the "invalid schema type" and "empty schema" steps)
use the raw key parameter and should use the test-unique helper like other
create steps; update both handlers to pass uniqueName(key) (or the project's
equivalent uniqueName function used in the other steps) instead of key so keys
are unique across runs and avoid test pollution or collisions during parallel
execution.
In `@tests/cucumber/step_definitions/dimension_steps.ts`:
- Around line 12-14: The helper function nextPosition() currently hardcodes and
returns 1 with a terse comment; update the comment above the nextPosition()
function to clearly document why position 1 is chosen (e.g., to always insert at
the front to prevent exceeding a max dimension count), the assumptions it makes
about test data ordering, and any potential impact on tests that rely on
dimension ordering or stable positions so future readers know the rationale and
when to change this behavior.
In `@tests/cucumber/step_definitions/experiment_steps.ts`:
- Around line 123-176: The step definitions use untyped ad-hoc fields via (this
as any)._pendingExpName/_pendingExpContext/_pendingExpVariants which bypasses
TypeScript checks; declare these fields on the SuperpositionWorld class (e.g.
public _pendingExpName: string = ""; public _pendingExpContext:
Record<string,string> = {}; public _pendingExpVariants: any[] = []; or a proper
Variant[] type) and then update the When handlers in experiment_steps.ts to
reference this._pendingExpName, this._pendingExpContext and
this._pendingExpVariants without casting to any; this ensures compile-time
safety and avoids silent runtime undefineds.
In `@tests/cucumber/step_definitions/function_steps.ts`:
- Around line 175-192: The step handler for When("I get function {string}", ...)
accepts the parameter name but ignores it and always uses this.functionName;
update the handler in the async function to use the step parameter by either
passing name into GetFunctionCommand (replace function_name: this.functionName
with function_name: name) or assign this.functionName = name before calling
this.client.send so the tracked name is updated; ensure GetFunctionCommand is
invoked with the correct variable and that any downstream assertions rely on the
chosen approach (direct use of name or updated this.functionName).
- Around line 244-264: The step handler in When("I publish function {string}",
...) uses a fragile magic string comparison (name === "non-existent-function")
to decide whether to pass the literal name into PublishCommand; update the logic
to remove the magic string by either adding a separate explicit step for failure
cases (e.g. "I publish a non-existent function") or by using an explicit
flag/property on the world (e.g. this.publishNonExistent) that the step checks
instead of the string; modify the PublishCommand call in this handler to use the
resolved funcName or the explicit flag to decide whether to pass the literal
non-existent name, and adjust any feature steps to use the new explicit step or
set the flag so behavior is not tied to a specific string literal.
In `@tests/cucumber/step_definitions/resolve_config_steps.ts`:
- Around line 1-14: The file has unused imports: DiscardExperimentCommand,
DeleteDefaultConfigCommand, and DeleteDimensionCommand are imported but never
referenced; remove these three symbols from the import list at the top (the
import that currently includes GetResolvedConfigWithIdentifierCommand,
CreateDimensionCommand, CreateDefaultConfigCommand, CreateExperimentCommand,
RampExperimentCommand, DiscardExperimentCommand, DeleteDefaultConfigCommand,
DeleteDimensionCommand, VariantType) so only actually used commands remain
imported.
- Around line 25-64: The catch blocks in the Given step that wrap
this.client.send(CreateDimensionCommand(...)) and
this.client.send(CreateDefaultConfigCommand(...)) currently swallow all errors;
change them to inspect the caught error (from the promise rejection) and only
suppress it when it clearly indicates an "already exists" condition (e.g., check
error.code or error.message), otherwise log the full error via
processLogger/errorLogger or rethrow so test failures surface; reference the
CreateDimensionCommand and CreateDefaultConfigCommand calls and the
this.createdDimensions / this.createdConfigs mutation sites when making the
change.
In `@tests/cucumber/step_definitions/secret_steps.ts`:
- Around line 184-210: Duplicate step definitions exist for the "I test the
compute function" step; remove the redundant When(...) block and instead update
the existing When handler for "I test the compute function" (the step using
SuperpositionWorld, TestCommand and this.client.send that sets
this.lastResponse/this.lastError) to accept both forms by using a single pattern
that allows an optional " again" (so both "I test the compute function" and "I
test the compute function again" map to the same handler). Delete the duplicate
handler and keep the shared implementation in the original When(...) that
constructs TestCommand and sets this.lastResponse/this.lastError.
In `@tests/cucumber/step_definitions/type_template_steps.ts`:
- Around line 125-145: The step handler ignores the incoming name parameter and
always uses this.typeTemplateName; update the When step so
DeleteTypeTemplatesCommand is constructed with type_name: name (not
this.typeTemplateName), and also use name when filtering
this.createdTypeTemplates (i.e., remove the passed-in name) so
DeleteTypeTemplatesCommand, the createdTypeTemplates filter, and any related
state updates consistently operate on the provided name parameter.
- Around line 103-123: The step handler for the When step is ignoring the
provided `name` parameter; update the UpdateTypeTemplatesCommand call in the
async function (the step definition in type_template_steps.ts) to use the `name`
argument instead of `this.typeTemplateName` for the `type_name` field (or
alternatively remove the `{string}` parameter from the step pattern if you
prefer not to accept a name); specifically change the `type_name` property in
the command passed to `this.client.send` within the When handler so it uses
`name`.
In `@tests/cucumber/support_ui/world.ts`:
- Around line 55-59: PlaywrightWorld is missing typed pending experiment fields
so the multi-step experiment creation uses (this as any) and loses compile-time
checks; add typed properties to the PlaywrightWorld class for _pendingExpName
(string | undefined), _pendingExpContext (object | undefined, e.g.,
Record<string, unknown>), and _pendingExpVariants (array, e.g., any[] or a
specific Variant type) and initialize them (e.g., undefined or empty array) next
to the existing experiment state (experimentId, experimentVariants,
createdExperimentIds) so experiment_steps.ts can use this._pendingExpName /
this._pendingExpContext / this._pendingExpVariants without casting to any.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+46
to
+72
| for (const fn of functions) { | ||
| try { | ||
| await this.client.send( | ||
| new CreateFunctionCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| function_name: fn.name, | ||
| function: fn.code, | ||
| description: `Test ${fn.type} function`, | ||
| change_reason: "Cucumber test setup", | ||
| runtime_version: FunctionRuntimeVersion.V1, | ||
| function_type: fn.type, | ||
| }) | ||
| ); | ||
| this.createdFunctions.push(fn.name); | ||
|
|
||
| await this.client.send( | ||
| new PublishCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| function_name: fn.name, | ||
| change_reason: "Publishing for cucumber tests", | ||
| }) | ||
| ); | ||
| } catch { | ||
| // Already exists | ||
| } |
Contributor
There was a problem hiding this comment.
Always attempt PublishCommand for the shared helper functions.
Right now publish only runs when CreateFunctionCommand succeeds. If a previous run already created false_validation, true_function, or auto_fn but left it unpublished, this Given step swallows the create error and never republishes it, so later config scenarios depend on stale external state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/default_config_steps.ts` around lines 46 -
72, The Create/Publish flow currently skips publishing if CreateFunctionCommand
throws; modify the logic around CreateFunctionCommand and PublishCommand so
PublishCommand is attempted regardless of whether CreateFunctionCommand
succeeded: call this.client.send(new PublishCommand(...)) in a finally block (or
after catching create errors) for each fn so shared helpers like
false_validation/true_function/auto_fn get republished; still push fn.name into
this.createdFunctions only when creation succeeds (or track published names
separately) and ensure any errors from PublishCommand are handled/logged but do
not prevent the loop from continuing.
Comment on lines
+249
to
+348
| When( | ||
| "I create a default config with key {string} and an invalid schema type {string}", | ||
| async function (this: PlaywrightWorld, key: string, schemaType: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new CreateDefaultConfigCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| key, | ||
| schema: { type: schemaType }, | ||
| value: { name: "Test" }, | ||
| description: "Test", | ||
| change_reason: "Testing invalid schema", | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| When( | ||
| "I create a default config with key {string} and an empty schema", | ||
| async function (this: PlaywrightWorld, key: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new CreateDefaultConfigCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| key, | ||
| schema: {}, | ||
| value: { name: "Test" }, | ||
| description: "Test", | ||
| change_reason: "Testing empty schema", | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| When( | ||
| "I create a default config with key {string} where age is {int} but minimum is {int}", | ||
| async function (this: PlaywrightWorld, key: string, age: number, min: number) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new CreateDefaultConfigCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| key, | ||
| schema: { | ||
| type: "object", | ||
| properties: { | ||
| name: { type: "string" }, | ||
| age: { type: "number", minimum: min }, | ||
| }, | ||
| required: ["name"], | ||
| }, | ||
| value: { name: "Test User", age }, | ||
| description: "Test", | ||
| change_reason: "Testing schema violation", | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| When( | ||
| "I create a default config with key {string} using validation function {string}", | ||
| async function (this: PlaywrightWorld, key: string, funcName: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new CreateDefaultConfigCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| key, | ||
| schema: { type: "object", properties: { name: { type: "string" } } }, | ||
| value: { name: "Invalid Value" }, | ||
| description: "Test", | ||
| value_validation_function_name: funcName, | ||
| change_reason: "Testing function validation", | ||
| }) | ||
| ); | ||
| this.createdConfigs.push(key); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Use unique keys for the negative create cases too.
These steps send the raw key into the shared workspace. If a leftover config with the same key already exists, the call fails with duplicate-key before the schema/function validation under test, so the scenario can pass for the wrong reason. Apply this.uniqueName(key) here as well, or delete any pre-existing key first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/default_config_steps.ts` around lines 249
- 348, The tests use raw key values which can collide with existing defaults and
cause false positives; update each failing-case step that calls
CreateDefaultConfigCommand (the blocks with "invalid schema type", "empty
schema", "age ... minimum", and "using validation function") to call
this.uniqueName(key) and use that returned uniqueKey when constructing the
command (and where applicable push it into this.createdConfigs) so the test
exercises schema/function validation rather than duplicate-key errors;
alternatively, remove any pre-existing config for the key before calling
this.client.send, but prefer using this.uniqueName(key) for consistency with
other steps.
Comment on lines
+21
to
+25
| // Track group-specific experiment IDs | ||
| let validExpId: string; | ||
| let validExp2Id: string; | ||
| let inProgressExpId: string; | ||
| let conflictingContextExpId: string; |
Contributor
There was a problem hiding this comment.
Move these experiment IDs onto PlaywrightWorld.
These module-scoped variables survive across scenarios, while the UI Before hook in tests/cucumber/support_ui/hooks.ts:193-220 only resets world fields. In parallel runs—or when a scenario skips the setup step—you can end up creating groups with stale or already-cleaned experiment IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts` around lines 21
- 25, The module-scoped experiment ID variables (validExpId, validExp2Id,
inProgressExpId, conflictingContextExpId) must be moved onto the PlaywrightWorld
instance so they are reset by the UI Before hook; remove the top-level
declarations and instead add these properties to the PlaywrightWorld type and
reference them via the world (e.g., world.validExpId, world.validExp2Id,
world.inProgressExpId, world.conflictingContextExpId) throughout
experiment_group_steps.ts; update any imports/uses to access the world object
(or this.playwrightWorld) and ensure the Before hook in
tests/cucumber/support_ui/hooks.ts continues to initialize/clear those world
fields.
Comment on lines
+752
to
+768
| Then( | ||
| "the list should contain the created group", | ||
| async function (this: PlaywrightWorld) { | ||
| const data = this.lastResponse?.data; | ||
| assert.ok(Array.isArray(data), "Response is not a list"); | ||
| assert.ok(data.length > 0, "List is empty"); | ||
|
|
||
| // Also verify the experiment groups page shows rows | ||
| try { | ||
| await this.goToWorkspacePage("experiment-groups"); | ||
| await this.page.waitForTimeout(500); | ||
| const rowCount = await this.page.locator("table tbody tr").count(); | ||
| assert.ok(rowCount > 0, "Experiment groups table has no rows in the UI"); | ||
| } catch { | ||
| // UI verification is best-effort | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Actually verify that the created group is in the list.
This only asserts “non-empty list” plus “UI table has rows”. If stale groups already exist, the step passes even when this.experimentGroupId never appears in the response.
Suggested fix
async function (this: PlaywrightWorld) {
const data = this.lastResponse?.data;
assert.ok(Array.isArray(data), "Response is not a list");
assert.ok(data.length > 0, "List is empty");
+ assert.ok(
+ data.some((group: any) => group.id === this.experimentGroupId),
+ `Experiment group ${this.experimentGroupId} not found in list`
+ );
// Also verify the experiment groups page shows rows
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts` around lines
752 - 768, Update the step "the list should contain the created group" to assert
the created group's presence rather than just non-empty lists: check that
this.lastResponse?.data is an array and that it includes an item whose id
matches this.experimentGroupId (e.g., find a record with id ===
this.experimentGroupId in the array), and in the UI block after calling
this.goToWorkspacePage("experiment-groups") replace the generic row count check
with a targeted locator that looks for a table row containing the experiment
group identifier (use this.page.locator with a text/has-text match for
this.experimentGroupId) and assert that its count is > 0; keep the UI check as
best-effort (inside the existing try/catch).
Comment on lines
+226
to
+248
| When( | ||
| "I get function {string}", | ||
| async function (this: PlaywrightWorld, name: string) { | ||
| try { | ||
| // Navigate to the function detail page | ||
| await this.goToDetailPage("function", this.functionName); | ||
| await this.page.waitForTimeout(300); | ||
|
|
||
| // Fetch via SDK for response assertions | ||
| this.lastResponse = await this.client.send( | ||
| new GetFunctionCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| function_name: this.functionName, | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Bug: name parameter is ignored - always uses this.functionName.
The step definition accepts a name parameter from the Gherkin step but completely ignores it, always using this.functionName for both navigation and SDK calls. This could cause tests to pass incorrectly when this.functionName differs from the name specified in the feature file.
🐛 Proposed fix
When(
"I get function {string}",
async function (this: PlaywrightWorld, name: string) {
+ // Use the stored functionName if it matches, otherwise use the provided name
+ const funcName = this.functionName === this.uniqueName(name) ? this.functionName : name;
try {
// Navigate to the function detail page
- await this.goToDetailPage("function", this.functionName);
+ await this.goToDetailPage("function", funcName);
await this.page.waitForTimeout(300);
// Fetch via SDK for response assertions
this.lastResponse = await this.client.send(
new GetFunctionCommand({
workspace_id: this.workspaceId,
org_id: this.orgId,
- function_name: this.functionName,
+ function_name: funcName,
})
);
this.lastError = undefined;
} catch (e: any) {
this.lastError = e;
this.lastResponse = undefined;
}
}
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/function_steps.ts` around lines 226 - 248,
The step handler for When("I get function {string}") is ignoring the incoming
name parameter and always using this.functionName; update the implementation in
the When(...) async function to use the local name variable for navigation and
the SDK call (replace this.functionName with name in calls to
this.goToDetailPage and in the new GetFunctionCommand payload), and optionally
set this.functionName = name if subsequent steps expect the world state to
reflect the requested function.
Comment on lines
+21
to
+44
| Given( | ||
| "a workspace exists with name {string}", | ||
| async function (this: SuperpositionWorld, name: string) { | ||
| const uniqueName = this.uniqueName(name); | ||
| try { | ||
| const response = await this.client.send( | ||
| new CreateWorkspaceCommand({ | ||
| org_id: this.orgId, | ||
| workspace_admin_email: "admin@example.com", | ||
| workspace_name: uniqueName, | ||
| workspace_status: WorkspaceStatus.ENABLED, | ||
| allow_experiment_self_approval: true, | ||
| auto_populate_control: false, | ||
| enable_context_validation: true, | ||
| enable_change_reason_validation: true, | ||
| }) | ||
| ); | ||
| this.workspaceName = uniqueName; | ||
| } catch { | ||
| // May already exist | ||
| this.workspaceName = uniqueName; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if workspace API uses workspace_name as the ID or returns a separate ID field
rg -n 'workspace_id|workspaceId' tests/cucumber/step_definitions/workspace_steps.ts tests/cucumber/support/world.ts -A2 -B2Repository: juspay/superposition
Length of output: 333
🏁 Script executed:
# Find CreateWorkspaceCommand definition and response structure
rg -n 'CreateWorkspaceCommand' tests/cucumber/step_definitions/workspace_steps.ts -A 20 | head -60Repository: juspay/superposition
Length of output: 2025
🏁 Script executed:
# Check how workspaceId is used in other steps - look for assignments and API calls
rg -n 'this\.workspaceId\s*=' tests/cucumber/step_definitions/workspace_steps.ts -B 2 -A 2Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look for other places where workspace is created and workspaceId is set
rg -n 'workspaceId' tests/cucumber/step_definitions/workspace_steps.ts -B 3 -A 3Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Look at the Before hook in world.ts to see how initial workspace is set up
rg -n 'Before' tests/cucumber/support/world.ts -A 30Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Search for how responses are used after CreateWorkspaceCommand
rg -n 'response\.' tests/cucumber/step_definitions/workspace_steps.ts -B 2 -A 2Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Check if there's a workspace_id field in any responses
rg -n 'workspace_id' tests/cucumber/step_definitions/workspace_steps.ts -B 3 -A 3Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
# Get file sizes first
wc -l tests/cucumber/support/world.ts tests/cucumber/step_definitions/workspace_steps.tsRepository: juspay/superposition
Length of output: 167
🏁 Script executed:
# Read world.ts to understand initialization and workspaceId usage
cat -n tests/cucumber/support/world.tsRepository: juspay/superposition
Length of output: 3438
🏁 Script executed:
# Read the entire workspace_steps.ts file
cat -n tests/cucumber/step_definitions/workspace_steps.tsRepository: juspay/superposition
Length of output: 7933
🏁 Script executed:
# Check if workspaceId is actually used anywhere in the step definitions or world
rg 'workspaceId' tests/cucumber/Repository: juspay/superposition
Length of output: 27167
🏁 Script executed:
# Check what fields are in the response from CreateWorkspaceCommand
rg -n 'lastResponse\.' tests/cucumber/step_definitions/workspace_steps.tsRepository: juspay/superposition
Length of output: 578
🏁 Script executed:
# Check the actual test feature files to understand how workspaces are used
find tests -name "*.feature" -type fRepository: juspay/superposition
Length of output: 624
🏁 Script executed:
# Read the hooks.ts file to see how workspaceId is initialized and what sharedWorkspaceId is
cat -n tests/cucumber/support/hooks.tsRepository: juspay/superposition
Length of output: 9001
🏁 Script executed:
# Check if the response from CreateWorkspaceCommand contains workspace_id or if workspace_name is used as ID
rg -n 'workspace_name|workspace_id' tests/cucumber/step_definitions/config_steps.ts -B 2 -A 2 | head -40Repository: juspay/superposition
Length of output: 1407
🏁 Script executed:
# Check how CreateWorkspaceCommand response is structured or used elsewhere
rg -n 'CreateWorkspaceCommand' tests/cucumber/support/hooks.ts -A 15 | grep -A 15 'await client.send'Repository: juspay/superposition
Length of output: 90
🏁 Script executed:
# Look at the feature file to understand the test flow
cat -n tests/cucumber/features/workspace.featureRepository: juspay/superposition
Length of output: 2912
Missing workspaceId update after creating workspace in the Given step.
This step creates a workspace and sets workspaceName but never updates this.workspaceId, which is used by all other steps for API calls. Subsequent steps will continue using the shared "cucumberws" workspace from the Before hook instead of the newly created workspace, breaking test isolation.
Additionally, the "I list workspaces filtered by status" step (lines 110-127) accepts a status parameter but never passes it to the ListWorkspaceCommand.
🐛 Proposed fixes
For the workspaceId issue (lines 21-44):
Given(
"a workspace exists with name {string}",
async function (this: SuperpositionWorld, name: string) {
const uniqueName = this.uniqueName(name);
try {
const response = await this.client.send(
new CreateWorkspaceCommand({
org_id: this.orgId,
workspace_admin_email: "admin@example.com",
workspace_name: uniqueName,
workspace_status: WorkspaceStatus.ENABLED,
allow_experiment_self_approval: true,
auto_populate_control: false,
enable_context_validation: true,
enable_change_reason_validation: true,
})
);
this.workspaceName = uniqueName;
+ this.workspaceId = uniqueName;
} catch {
// May already exist
this.workspaceName = uniqueName;
+ this.workspaceId = uniqueName;
}
}
);For the status filter issue (lines 110-127):
When(
"I list workspaces filtered by status {string}",
async function (this: SuperpositionWorld, status: string) {
try {
this.lastResponse = await this.client.send(
new ListWorkspaceCommand({
count: 5,
page: 1,
org_id: this.orgId,
+ status: status,
})
);📝 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.
Suggested change
| Given( | |
| "a workspace exists with name {string}", | |
| async function (this: SuperpositionWorld, name: string) { | |
| const uniqueName = this.uniqueName(name); | |
| try { | |
| const response = await this.client.send( | |
| new CreateWorkspaceCommand({ | |
| org_id: this.orgId, | |
| workspace_admin_email: "admin@example.com", | |
| workspace_name: uniqueName, | |
| workspace_status: WorkspaceStatus.ENABLED, | |
| allow_experiment_self_approval: true, | |
| auto_populate_control: false, | |
| enable_context_validation: true, | |
| enable_change_reason_validation: true, | |
| }) | |
| ); | |
| this.workspaceName = uniqueName; | |
| } catch { | |
| // May already exist | |
| this.workspaceName = uniqueName; | |
| } | |
| } | |
| ); | |
| Given( | |
| "a workspace exists with name {string}", | |
| async function (this: SuperpositionWorld, name: string) { | |
| const uniqueName = this.uniqueName(name); | |
| try { | |
| const response = await this.client.send( | |
| new CreateWorkspaceCommand({ | |
| org_id: this.orgId, | |
| workspace_admin_email: "admin@example.com", | |
| workspace_name: uniqueName, | |
| workspace_status: WorkspaceStatus.ENABLED, | |
| allow_experiment_self_approval: true, | |
| auto_populate_control: false, | |
| enable_context_validation: true, | |
| enable_change_reason_validation: true, | |
| }) | |
| ); | |
| this.workspaceName = uniqueName; | |
| this.workspaceId = uniqueName; | |
| } catch { | |
| // May already exist | |
| this.workspaceName = uniqueName; | |
| this.workspaceId = uniqueName; | |
| } | |
| } | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/workspace_steps.ts` around lines 21 - 44, The
Given step that calls CreateWorkspaceCommand sets this.workspaceName but never
assigns this.workspaceId from the response—capture the created workspace's id
from the CreateWorkspaceCommand response and set this.workspaceId so subsequent
steps use the new workspace; also update the "I list workspaces filtered by
status" step to pass the incoming status parameter into the ListWorkspaceCommand
request so the API call actually filters by the provided WorkspaceStatus. Use
the existing CreateWorkspaceCommand and ListWorkspaceCommand symbols and update
assignments to this.workspaceId and the ListWorkspaceCommand payload
respectively.
Comment on lines
+63
to
+87
| When( | ||
| "I create a workspace with name {string} and admin email {string}", | ||
| async function (this: SuperpositionWorld, name: string, email: string) { | ||
| const uniqueName = name ? this.uniqueName(name) : name; | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new CreateWorkspaceCommand({ | ||
| org_id: this.orgId, | ||
| workspace_admin_email: email, | ||
| workspace_name: uniqueName, | ||
| workspace_status: WorkspaceStatus.ENABLED, | ||
| allow_experiment_self_approval: true, | ||
| auto_populate_control: false, | ||
| enable_context_validation: true, | ||
| enable_change_reason_validation: true, | ||
| }) | ||
| ); | ||
| this.workspaceName = uniqueName; | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Same issue: workspaceId not set after creation.
Similar to the Given step, the When step for creating a workspace sets workspaceName but not workspaceId, so subsequent operations won't target the newly created workspace.
🐛 Proposed fix
this.lastResponse = await this.client.send(
new CreateWorkspaceCommand({
org_id: this.orgId,
workspace_admin_email: email,
workspace_name: uniqueName,
workspace_status: WorkspaceStatus.ENABLED,
allow_experiment_self_approval: true,
auto_populate_control: false,
enable_context_validation: true,
enable_change_reason_validation: true,
})
);
this.workspaceName = uniqueName;
+ this.workspaceId = this.lastResponse.workspace_name ?? uniqueName;
this.lastError = undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/workspace_steps.ts` around lines 63 - 87, The
When step creates a workspace but never sets this.workspaceId, so update the
handler (the async function in workspace_steps.ts that calls
CreateWorkspaceCommand) to, after a successful response (this.lastResponse),
extract the created workspace's id from the response object (e.g., the workspace
id field on this.lastResponse or its payload) and assign it to this.workspaceId
(similar to how this.workspaceName is set); keep the existing try/catch behavior
and only set workspaceId when the CreateWorkspaceCommand call succeeds.
Comment on lines
+110
to
+127
| When( | ||
| "I list workspaces filtered by status {string}", | ||
| async function (this: SuperpositionWorld, status: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new ListWorkspaceCommand({ | ||
| count: 5, | ||
| page: 1, | ||
| org_id: this.orgId, | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ListWorkspaceCommand accepts a status parameter
ast-grep --pattern 'ListWorkspaceCommand'Repository: juspay/superposition
Length of output: 2208
🏁 Script executed:
#!/bin/bash
# Find the definition of ListWorkspaceCommand - it's likely in src/ directory
find . -type f -name "*.ts" -path "*/src/*" | head -20Repository: juspay/superposition
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Search for ListWorkspaceCommand definition/export
rg "class ListWorkspaceCommand|export.*ListWorkspaceCommand|interface.*ListWorkspaceCommand" -A 5Repository: juspay/superposition
Length of output: 3676
🏁 Script executed:
#!/bin/bash
# Check the codebase structure to understand where commands are defined
ls -la src/ 2>/dev/null || ls -la clients/Repository: juspay/superposition
Length of output: 577
🏁 Script executed:
#!/bin/bash
# Find ListWorkspaceInput definition to see what parameters are accepted
rg "interface ListWorkspaceInput|type ListWorkspaceInput" -A 20Repository: juspay/superposition
Length of output: 3789
🏁 Script executed:
#!/bin/bash
# Read the full ListWorkspaceCommand.ts file to understand the command structure
cat -n clients/javascript/sdk/src/commands/ListWorkspaceCommand.tsRepository: juspay/superposition
Length of output: 5394
🏁 Script executed:
#!/bin/bash
# Search for WorkspaceStatus type definition
rg "type WorkspaceStatus|interface WorkspaceStatus|enum WorkspaceStatus" -A 5Repository: juspay/superposition
Length of output: 1757
🏁 Script executed:
#!/bin/bash
# Check the step definition file to see what status values are being used
cat -n tests/cucumber/step_definitions/workspace_steps.ts | grep -A 30 "I list workspaces filtered by status"Repository: juspay/superposition
Length of output: 1098
🏁 Script executed:
#!/bin/bash
# Check if there's any filtering logic in the test step or elsewhere
rg "status.*filter|filter.*status" tests/cucumber/step_definitions/Repository: juspay/superposition
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Look for assertions that check workspace status in the feature files
rg "workspace.*status|status.*workspace" tests/cucumber/features/ -A 2 -B 2Repository: juspay/superposition
Length of output: 1128
🏁 Script executed:
#!/bin/bash
# Check the assertion step that validates workspace status
rg "all returned workspaces should have status" tests/cucumber/step_definitions/ -A 10Repository: juspay/superposition
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Also search for any status filtering logic in the responses
rg "workspace_status|workspace.*status" tests/cucumber/step_definitions/workspace_steps.ts -A 5 -B 2Repository: juspay/superposition
Length of output: 1827
🏁 Script executed:
#!/bin/bash
# Check the smithy API definition for ListWorkspaceInput to see if status filtering is defined
cat smithy/models/workspace.smithy | head -100Repository: juspay/superposition
Length of output: 2102
🏁 Script executed:
#!/bin/bash
# Search for ListWorkspaceInput in the smithy models
rg "operation ListWorkspace|input.*ListWorkspaceInput" smithy/models/Repository: juspay/superposition
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Get the full ListWorkspace operation definition
rg "operation ListWorkspace" smithy/models/workspace.smithy -A 20Repository: juspay/superposition
Length of output: 878
🏁 Script executed:
#!/bin/bash
# Search for ListWorkspaceInput structure definition
rg "structure ListWorkspaceInput|ListWorkspaceInput" smithy/models/workspace.smithy -B 2 -A 15Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check what PaginationParams and OrganisationMixin include
rg "mixin PaginationParams|structure PaginationParams" smithy/models/ -A 10Repository: juspay/superposition
Length of output: 724
🏁 Script executed:
#!/bin/bash
# Check OrganisationMixin
rg "mixin OrganisationMixin|structure OrganisationMixin" smithy/models/ -A 10Repository: juspay/superposition
Length of output: 520
Status filtering is not supported by the ListWorkspaceCommand API.
The step accepts a status parameter but the underlying ListWorkspaceCommand doesn't support status filtering—the ListWorkspaceInput interface only accepts count, page, all, and org_id. The proposed fix cannot work because the API lacks this parameter entirely.
Either extend the API to support status filtering in ListWorkspaceInput, or remove the status filtering scenario from the test since the current implementation retrieves all workspaces without filtering by status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/workspace_steps.ts` around lines 110 - 127,
The step handler currently declares a status parameter but calls
ListWorkspaceCommand which does not support status filtering (ListWorkspaceInput
only accepts count, page, all, org_id); either remove the unused status
parameter and any corresponding Gherkin scenario that expects status-filtered
results, or add status filtering support by extending the ListWorkspaceInput and
ListWorkspaceCommand to accept a status field and pass it through in the call
(update the step to include that status in the new input); locate the When step
in workspace_steps.ts and the ListWorkspaceCommand/ListWorkspaceInput types to
apply the chosen change.
Comment on lines
+21
to
+22
| const TEST_ORG_NAME = "cucumberorg"; | ||
| const TEST_WORKSPACE = "cucumberws"; |
Contributor
There was a problem hiding this comment.
Avoid reusing arbitrary cucumberorg* state.
This setup attaches to any organisation whose name starts with cucumberorg, and even falls back to the first prefix match when no “usable” workspace is found. On a reused/shared server, that can mutate stale data from another run and make cleanup delete resources this run never created. Please create a run-scoped org/workspace, or persist the exact IDs created by this run instead of prefix-matching existing tenants.
Also applies to: 36-77
Comment on lines
+137
to
+167
| After(async function (this: SuperpositionWorld) { | ||
| // Cleanup experiment groups | ||
| if (this.experimentGroupId) { | ||
| try { | ||
| await this.client.send( | ||
| new DeleteExperimentGroupCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| id: this.experimentGroupId, | ||
| }) | ||
| ); | ||
| } catch { | ||
| // May have members or already deleted | ||
| } | ||
| } | ||
|
|
||
| // Cleanup experiments | ||
| for (const id of this.createdExperimentIds) { | ||
| try { | ||
| await this.client.send( | ||
| new DiscardExperimentCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| id, | ||
| change_reason: "Cucumber cleanup", | ||
| }) | ||
| ); | ||
| } catch { | ||
| // Already discarded or concluded | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Delete member experiments before deleting the group.
The first cleanup attempt deletes the experiment group before discarding its members, but the catch already acknowledges that the delete can fail while members still exist. Because the group delete is never retried after the experiments are discarded, groups with members leak across scenarios and can pollute later list/quota tests.
Suggested fix
- // Cleanup experiment groups
- if (this.experimentGroupId) {
- try {
- await this.client.send(
- new DeleteExperimentGroupCommand({
- workspace_id: this.workspaceId,
- org_id: this.orgId,
- id: this.experimentGroupId,
- })
- );
- } catch {
- // May have members or already deleted
- }
- }
-
// Cleanup experiments
for (const id of this.createdExperimentIds) {
try {
await this.client.send(
new DiscardExperimentCommand({
@@
} catch {
// Already discarded or concluded
}
}
+
+ // Cleanup experiment groups after member experiments are gone
+ if (this.experimentGroupId) {
+ try {
+ await this.client.send(
+ new DeleteExperimentGroupCommand({
+ workspace_id: this.workspaceId,
+ org_id: this.orgId,
+ id: this.experimentGroupId,
+ })
+ );
+ } catch {
+ // May already be deleted
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/support/hooks.ts` around lines 137 - 167, The cleanup
currently calls DeleteExperimentGroupCommand before discarding member
experiments, causing group deletes to fail and leak; change After(this:
SuperpositionWorld) so you first iterate this.createdExperimentIds and send
DiscardExperimentCommand for each (using client.send), then if
this.experimentGroupId is set attempt DeleteExperimentGroupCommand afterwards;
additionally, if DeleteExperimentGroupCommand fails due to members, retry the
delete once after discards (or log the error with context) to ensure groups are
removed; reference the symbols After, SuperpositionWorld,
this.createdExperimentIds, this.experimentGroupId, DiscardExperimentCommand,
DeleteExperimentGroupCommand, and client.send when locating code to modify.
Root cause: Leptos Button components show loading spinners until WASM hydration completes (client_side_ready). ChangeLogPopup confirm buttons also start disabled until their resource loads. Tests were clicking buttons before they became interactive. Fixes: - Add waitForHydration() to all navigation helpers - waits for loading-dots spinners to disappear before interacting with page - Add waitForConfirmButton() helper that waits for confirm dialog buttons to be visible AND enabled before clicking - Use waitForConfirmButton() for all "Yes, Delete"/"Yes, Update" clicks - Use #workspace_drawer-btn ID selector instead of CSS class for workspace create button (more reliable) https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
tests/cucumber/features/workspace.feature-19-38 (1)
19-38:⚠️ Potential issue | 🟡 MinorPotential test isolation issue with shared workspace name.
Multiple scenarios use the hardcoded name
"cucumbertestws":
- Line 20: Create scenario creates it
- Line 29: "Find a created workspace" expects it to exist
- Line 36: Update scenario uses it
If scenarios run in parallel (as noted in PR objectives as a potential future issue), these tests may interfere with each other. Consider using unique names per scenario or ensuring explicit dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/features/workspace.feature` around lines 19 - 38, Multiple scenarios share the hardcoded workspace name "cucumbertestws" (used in the "Create a new workspace", "Find a created workspace in the list", and "Update workspace admin email" scenarios), which can cause test isolation and parallel-run flakiness; update the feature to generate or inject unique workspace names per scenario (e.g., append a timestamp/UUID or use Cucumber placeholders/Scenario Outline with a background hook that creates a workspace and stores its name) or add explicit setup/teardown hooks so each scenario uses its own workspace instead of the shared "cucumbertestws" string.tests/cucumber/step_definitions/variable_steps.ts-33-35 (1)
33-35:⚠️ Potential issue | 🟡 MinorSilent catch block may mask unexpected errors.
This catch block silently ignores all errors, not just "already exists" scenarios. A database connection error, authentication failure, or other unexpected error would be silently swallowed, potentially causing confusing test failures downstream.
🛡️ Suggested improvement to handle specific errors
} catch (e: any) { - // Already exists + // Tolerate "already exists" errors; re-throw unexpected ones + const msg = e.message || String(e); + if (!msg.includes("duplicate key") && !msg.includes("already exists")) { + throw e; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/variable_steps.ts` around lines 33 - 35, The catch block in tests/cucumber/step_definitions/variable_steps.ts currently swallows all errors; change it to only ignore the "already exists" case by inspecting the caught error (e.g., check error.message or error.code for the "already exists" indicator) and return silently in that specific case, but for any other error log the error (or call processLogger/error/assert) and rethrow or fail the test so unexpected DB/connection/auth errors are not masked; locate the try/catch around the variable creation call in variable_steps.ts and implement the conditional handling there.tests/cucumber/features/experiment_group.feature-7-10 (1)
7-10:⚠️ Potential issue | 🟡 MinorBackground setup creates shared state that may conflict in parallel execution.
Per the step implementation (context snippet 3), experiment IDs are stored in module-level variables (
validExpId,validExp2Id, etc.) rather than world state. This works for sequential execution but will cause race conditions if scenarios run in parallel, as noted in the PR objectives under "Potential future issues."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/features/experiment_group.feature` around lines 7 - 10, Tests use module-level variables (validExpId, validExp2Id, etc.) for experiment IDs which creates shared state and race conditions; change them to be stored on the Cucumber World (or the provided context object) instead. Update the step implementation for "Given experiments are set up for group tests" and any other step definitions referencing validExpId/validExp2Id to assign and read from this.validExpId / this.validExp2Id (or context.validExpId) rather than module-level vars, and adjust imports/usages throughout the related step files so all steps access the IDs from the World instance to isolate state per scenario. Ensure any teardown or reuse also reads/writes from the World properties.tests/cucumber/features/organisation.feature-50-52 (1)
50-52:⚠️ Potential issue | 🟡 MinorTest may fail for wrong reason:
page 0might also be invalid.If page must be > 0 (as tested in line 47-48), then
page 0in this scenario is also invalid. The test might fail due to the page validation rather than the count validation, making the test assertion misleading.Proposed fix
Scenario: Fail to list organisations with negative count - When I list organisations with count -1 and page 0 + When I list organisations with count -1 and page 1 Then the operation should fail with error matching "Count should be greater than 0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/features/organisation.feature` around lines 50 - 52, The scenario "Fail to list organisations with negative count" uses "When I list organisations with count -1 and page 0" which may trigger page validation first; update the step to use a valid page (e.g., change page 0 to page 1) so the test only exercises the negative count validation in the failing case for the step "I list organisations with count -1 and page 0" within that scenario.tests/cucumber/step_definitions/type_template_steps.ts-103-123 (1)
103-123:⚠️ Potential issue | 🟡 MinorStep parameter
nameis ignored.The step accepts a
nameparameter but usesthis.typeTemplateNameinstead. This mirrors the pattern seen infunction_steps.ts.Consider either removing the parameter from the step pattern or documenting that it refers to the previously created template.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/type_template_steps.ts` around lines 103 - 123, The step definition in type_template_steps.ts declares a step parameter `name` but ignores it and uses the instance field `this.typeTemplateName`; update the step implementation to use the passed-in `name` for the UpdateTypeTemplatesCommand (set type_name: name) or alternatively remove the `{string}` parameter from the step pattern so it matches the intended usage; ensure any references to `this.typeTemplateName` are updated only if you choose to keep the field-based behavior so the step parameter isn't left unused.tests/cucumber/step_definitions/context_steps.ts-310-317 (1)
310-317:⚠️ Potential issue | 🟡 MinorOverride assertion doesn't validate the specific config key.
The step accepts a
configKeyparameter but only checks that any override exists, not that the specific key is present.🐛 Proposed fix to validate specific key
Then( "the response should include the override for {string}", function (this: SuperpositionWorld, configKey: string) { assert.ok(this.lastResponse, "No response"); const override = this.lastResponse.override ?? this.lastResponse.r_override; assert.ok(override, "No override in response"); + assert.ok( + configKey in override, + `Override for key "${configKey}" not found in response` + ); } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/context_steps.ts` around lines 310 - 317, The step definition for Then("the response should include the override for {string}") currently only checks that some override exists; update it to assert that the specific config key exists in the override object: retrieve the override object from this.lastResponse (using the existing override ?? r_override logic), ensure it's an object, then assert that override[configKey] !== undefined (or hasOwnProperty(configKey)) and fail with a clear message including configKey if missing; reference the Then(...) step, SuperpositionWorld.this.lastResponse and the local variable override to locate where to add the key-specific assertion.tests/cucumber/support/hooks.ts-21-22 (1)
21-22:⚠️ Potential issue | 🟡 MinorPotential race condition with UI tests using identical org/workspace.
The API hooks use
TEST_ORG_NAME = "cucumberorg"andTEST_WORKSPACE = "cucumberws". Per the context snippet fromsupport_ui/hooks.ts, the UI tests use identical constants. If API and UI tests run concurrently (e.g., parallel CI jobs), they may conflict when creating/modifying the same org/workspace.Consider using profile-specific prefixes:
-const TEST_ORG_NAME = "cucumberorg"; -const TEST_WORKSPACE = "cucumberws"; +const TEST_ORG_NAME = "cucumberorg-api"; +const TEST_WORKSPACE = "cucumberws-api";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/support/hooks.ts` around lines 21 - 22, The constants TEST_ORG_NAME and TEST_WORKSPACE are shared with the UI tests and can collide in parallel runs; change their definitions to include a profile-specific or run-unique suffix/prefix (e.g., use a TEST_PROFILE or CI_JOB_ID env var or a short random/idempotent timestamp) so names become deterministic-per-run, and update the matching definitions in support_ui/hooks.ts to use the same derivation function; modify the symbols TEST_ORG_NAME and TEST_WORKSPACE to be computed (via a helper like buildTestName(profile, base)) rather than hardcoded strings so API and UI tests won’t race.tests/cucumber/step_definitions/function_steps.ts-276-282 (1)
276-282:⚠️ Potential issue | 🟡 MinorStep parameter
nameis ignored in assertion.Similar to the get step, this assertion accepts a
nameparameter but always compares againstthis.functionName.🐛 Proposed fix
Then( "the response should have function name {string}", function (this: SuperpositionWorld, name: string) { assert.ok(this.lastResponse, "No response"); - assert.strictEqual(this.lastResponse.function_name, this.functionName); + // Verify the function name matches the tracked unique name (which is derived from the input name) + assert.ok( + this.lastResponse.function_name === this.functionName, + `Expected function name "${this.functionName}", got "${this.lastResponse.function_name}"` + ); } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/function_steps.ts` around lines 276 - 282, The step definition for Then("the response should have function name {string}", function (this: SuperpositionWorld, name: string) {...}) ignores the `name` parameter and compares to `this.functionName`; update the assertion in that handler to compare this.lastResponse.function_name to the `name` parameter (while keeping the existing null-check assert.ok(this.lastResponse, "No response")) so the step actually validates the supplied string rather than the instance property.tests/cucumber/step_definitions/function_steps.ts-175-192 (1)
175-192:⚠️ Potential issue | 🟡 MinorStep parameter
nameis ignored.The step definition accepts a
nameparameter from the Gherkin step but usesthis.functionNameinstead. This can cause confusion when reading feature files.🐛 Proposed fix to use parameter or remove it
Option 1 - Use the parameter:
When( "I get function {string}", async function (this: SuperpositionWorld, name: string) { + const funcName = this.functionName || this.uniqueName(name); try { this.lastResponse = await this.client.send( new GetFunctionCommand({ workspace_id: this.workspaceId, org_id: this.orgId, - function_name: this.functionName, + function_name: funcName, }) );Option 2 - Change step pattern:
- "I get function {string}", - async function (this: SuperpositionWorld, name: string) { + "I get the created function", + async function (this: SuperpositionWorld) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/function_steps.ts` around lines 175 - 192, The step handler declared as When(... async function (this: SuperpositionWorld, name: string)) ignores the passed-in parameter by using this.functionName when constructing the GetFunctionCommand; either use the step parameter `name` in the payload (replace `function_name: this.functionName` with `function_name: name`) so the Gherkin argument is honored, or remove the `name` parameter from the step signature and step pattern so the code intentionally relies on `this.functionName`; locate this code around the When(...) block and the GetFunctionCommand call to apply the chosen change.tests/cucumber/support/hooks.ts-130-135 (1)
130-135:⚠️ Potential issue | 🟡 MinorResource tracking arrays are not reset between scenarios.
The
Beforehook resetslastResponseandlastErrorbut doesn't reset tracking arrays likecreatedDimensions,createdFunctions,createdSecrets, etc. While theAfterhook iterates and deletes these resources, the arrays themselves persist.This could cause issues if a cleanup fails silently and subsequent scenarios accumulate stale references.
🛡️ Proposed fix to reset tracking arrays
Before(function (this: SuperpositionWorld) { this.orgId = sharedOrgId; this.workspaceId = sharedWorkspaceId; this.lastResponse = undefined; this.lastError = undefined; + // Reset resource tracking arrays for clean scenario isolation + this.createdDimensions = []; + this.createdConfigs = []; + this.createdContextIds = []; + this.createdExperimentIds = []; + this.createdFunctions = []; + this.createdVariables = []; + this.createdSecrets = []; + this.createdTypeTemplates = []; + this.experimentGroupId = ""; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/support/hooks.ts` around lines 130 - 135, The Before hook currently resets only orgId, workspaceId, lastResponse, and lastError; update it to also reinitialize all resource-tracking arrays on the SuperpositionWorld (e.g., createdDimensions, createdFunctions, createdSecrets, createdWorkspaces, createdProjects, etc.) so each scenario starts with empty arrays; locate the Before(function (this: SuperpositionWorld) { ... }) and add assignments that set each tracking property to an empty array to avoid carrying stale references between scenarios and complement the cleanup performed in the After hook.tests/cucumber/step_definitions/secret_steps.ts-39-66 (1)
39-66:⚠️ Potential issue | 🟡 MinorPotential code injection in dynamically generated function.
Line 45 uses
SECRETS.${secretName}in a template literal that becomes executable code. IfsecretNamecontains characters like]or newlines, it could break or alter the function's behavior.🛡️ Proposed fix to sanitize input
Given( "a compute function exists that reads the secret {string}", async function (this: SuperpositionWorld, secretName: string) { this.functionName = this.uniqueName("verify_secret"); + // Sanitize secret name to ensure it's a valid JS identifier + const sanitizedSecretName = secretName.replace(/[^a-zA-Z0-9_]/g, '_'); const code = ` async function execute(payload) { - return [SECRETS.${secretName}]; + return [SECRETS.${sanitizedSecretName}]; } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/secret_steps.ts` around lines 39 - 66, The template builds executable code with unescaped SECRETS.${secretName}, which can lead to code injection; change the generated code in the Given step (the code variable used when calling CreateFunctionCommand in this step definition) to access the secret via bracket notation with a safely escaped string literal (e.g., SECRETS[JSON.stringify(secretName)] or equivalent escaping) or validate/sanitize secretName against an allowlist before interpolation, so the produced execute function uses SECRETS["safeName"] instead of injecting raw characters.tests/cucumber/step_definitions/experiment_group_steps.ts-21-27 (1)
21-27:⚠️ Potential issue | 🟡 MinorMove experiment fixture IDs to
SuperpositionWorldto avoid shared state pollution.The module-level
letbindings at lines 21-27 are shared across all scenarios in the same worker process. While the current Cucumber configuration runs sequentially without parallel workers or retries, this design is fragile: if--parallelis ever enabled or the configuration changes, different scenarios could overwrite these IDs and corrupt subsequent assertions.Store
validExpId,validExp2Id,inProgressExpId, andconflictingContextExpIdas properties on theSuperpositionWorldinstance (likeexperimentGroupIdon line 53) so each scenario gets its own isolated copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/experiment_group_steps.ts` around lines 21 - 27, The module-level let bindings validExpId, validExp2Id, inProgressExpId, and conflictingContextExpId cause shared state across scenarios; move these into the SuperpositionWorld instance as properties (e.g., this.validExpId, this.validExp2Id, this.inProgressExpId, this.conflictingContextExpId) alongside existing experimentGroupId so each scenario has its own isolated IDs, update any step definitions and helpers that reference the module-level variables to use the SuperpositionWorld properties instead, and remove the top-level declarations from experiment_group_steps.ts.tests/cucumber/step_definitions_ui/variable_steps.ts-16-36 (1)
16-36:⚠️ Potential issue | 🟡 MinorVariable name not made unique in Given step, unlike other step files.
This Given step uses the raw
nameparameter without callingthis.uniqueName(name), unlike dimension, config, and other Given steps. This could cause test isolation issues when running tests in parallel.Proposed fix for test isolation
Given( "a variable {string} exists with value {string}", async function (this: PlaywrightWorld, name: string, value: string) { - this.variableName = name; + const uniqueName = this.uniqueName(name); + this.variableName = uniqueName; try { await this.client.send( new CreateVariableCommand({ workspace_id: this.workspaceId, org_id: this.orgId, - name, + name: uniqueName, value, - description: `Test variable ${name}`, + description: `Test variable ${uniqueName}`, change_reason: "Cucumber setup", }) ); - this.createdVariables.push(name); + this.createdVariables.push(uniqueName); } catch { // Already exists } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/variable_steps.ts` around lines 16 - 36, The Given step in variable_steps.ts uses the raw name parameter which can collide in parallel tests; update the step to generate and use a unique name by calling this.uniqueName(name) before assigning this.variableName and before sending CreateVariableCommand, and push that unique name into this.createdVariables (instead of the raw name) so CreateVariableCommand, this.variableName, and this.createdVariables all reference the unique identifier.tests/cucumber/step_definitions/organisation_steps.ts-13-34 (1)
13-34:⚠️ Potential issue | 🟡 MinorOrganisation name not made unique, may cause test isolation issues.
Unlike other entity types (dimensions, configs, etc.), organisation creation doesn't use
this.uniqueName(name). This could cause conflicts when running tests in parallel or when tests are re-run without cleanup.Proposed fix for test isolation
Given( "an organisation exists with name {string} and admin email {string}", async function (this: SuperpositionWorld, name: string, email: string) { + const uniqueName = this.uniqueName(name); try { const response = await this.client.send( - new CreateOrganisationCommand({ admin_email: email, name }) + new CreateOrganisationCommand({ admin_email: email, name: uniqueName }) ); this.createdOrgId = response.id ?? ""; - this.orgName = name; + this.orgName = uniqueName; } catch (e: any) { // May already exist, try to find it const list = await this.client.send(new ListOrganisationCommand({})); - const existing = list.data?.find((o) => o.name === name); + const existing = list.data?.find((o) => o.name === uniqueName); if (existing) { this.createdOrgId = existing.id ?? ""; - this.orgName = name; + this.orgName = uniqueName; } else { throw e; } } } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/organisation_steps.ts` around lines 13 - 34, The organisation creation step should use a unique test name to avoid collisions: call this.uniqueName(name) to generate a uniqueName, use that uniqueName when constructing CreateOrganisationCommand and when matching in the ListOrganisationCommand fallback, and set this.orgName = uniqueName; keep assignment to this.createdOrgId the same (response.id or existing.id) and preserve the try/catch logic around CreateOrganisationCommand, ListOrganisationCommand, this.createdOrgId and this.orgName.tests/cucumber/step_definitions/default_config_steps.ts-139-144 (1)
139-144:⚠️ Potential issue | 🟡 MinorBlank table cells are coerced to
0here.
Number("")is0, so an empty Gherkin cell stops being empty or missing once this parser runs. That changes required-field and schema-negative scenarios.💡 Minimal fix
const rows = table.rowsHash(); const value: any = {}; for (const [k, v] of Object.entries(rows)) { - value[k] = isNaN(Number(v)) ? v : Number(v); + value[k] = v.trim() !== "" && !Number.isNaN(Number(v)) ? Number(v) : v; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions/default_config_steps.ts` around lines 139 - 144, The loop that builds value from rowsHash is converting empty strings to 0 because it uses Number(v); update the coercion so empty cells remain empty strings (or undefined) instead of being converted to 0: inside the async step (the function that uses rowsHash and builds value), check for v === '' (or v.trim() === '') first and preserve it, otherwise only coerce to Number(v) when Number(v) is a valid number (e.g., !Number.isNaN(Number(v))); keep the rest of the mapping logic intact and reference rowsHash, value, and Number(v) when locating where to change the code.tests/cucumber/support_ui/world.ts-124-129 (1)
124-129:⚠️ Potential issue | 🟡 MinorEncode
entityNamewhen building detail-page URLs.Free-form names containing spaces,
/,#, or?will navigate to the wrong route and fail the step before the real assertion runs.💡 Minimal fix
async goToDetailPage(section: string, entityName: string): Promise<void> { + const encodedName = encodeURIComponent(entityName); await this.page.goto( - `${this.appUrl}/admin/${this.orgId}/${this.workspaceId}/${section}/${entityName}` + `${this.appUrl}/admin/${this.orgId}/${this.workspaceId}/${section}/${encodedName}` ); await this.page.waitForLoadState("networkidle"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/support_ui/world.ts` around lines 124 - 129, The goToDetailPage helper builds a URL with entityName raw, which breaks for names containing spaces or reserved chars; encode entityName before navigation. Update the goToDetailPage method to URL-encode entityName (e.g., via encodeURIComponent or equivalent) when composing the string passed to this.page.goto so the path becomes `${this.appUrl}/admin/${this.orgId}/${this.workspaceId}/${section}/${encodedEntityName}` and keep the existing this.page.waitForLoadState("networkidle") logic unchanged.tests/cucumber/step_definitions_ui/function_steps.ts-56-63 (1)
56-63:⚠️ Potential issue | 🟡 MinorEscape
keybefore embedding it in generated source.A step value containing quotes, backslashes, or newlines will produce invalid JavaScript here and fail the test for the wrong reason.
JSON.stringify(key)is enough.💡 Minimal fix
async function (this: PlaywrightWorld, name: string, key: string) { const uniqueName = this.uniqueName(name); this.functionName = uniqueName; + const expectedKey = JSON.stringify(key); const code = ` async function execute(payload) { let value = payload.value_validate.value; let key = payload.value_validate.key; - if (key === "${key}" && value === "valid") return true; + if (key === ${expectedKey} && value === "valid") return true; return false; } `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cucumber/step_definitions_ui/function_steps.ts` around lines 56 - 63, The embedded key value in the generated source (the template assigned to the code variable that defines async function execute(payload) and uses the key variable) isn't escaped, so values with quotes/backslashes/newlines break the generated JS; fix by replacing the raw interpolation with a properly JSON-escaped string (e.g., use JSON.stringify(key) when constructing the template) so the injected key is safely quoted and escaped in the generated code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 568239fd-7de5-4a97-8e9a-a7b4d443b2e5
⛔ Files ignored due to path filters (1)
tests/cucumber/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
flake.nixmakefiletests/cucumber/.gitignoretests/cucumber/cucumber.jstests/cucumber/features/config.featuretests/cucumber/features/context.featuretests/cucumber/features/default_config.featuretests/cucumber/features/dimension.featuretests/cucumber/features/experiment.featuretests/cucumber/features/experiment_group.featuretests/cucumber/features/function.featuretests/cucumber/features/organisation.featuretests/cucumber/features/resolve_config.featuretests/cucumber/features/secret.featuretests/cucumber/features/type_template.featuretests/cucumber/features/variable.featuretests/cucumber/features/workspace.featuretests/cucumber/package.jsontests/cucumber/step_definitions/common_steps.tstests/cucumber/step_definitions/config_steps.tstests/cucumber/step_definitions/context_steps.tstests/cucumber/step_definitions/default_config_steps.tstests/cucumber/step_definitions/dimension_steps.tstests/cucumber/step_definitions/experiment_group_steps.tstests/cucumber/step_definitions/experiment_steps.tstests/cucumber/step_definitions/function_steps.tstests/cucumber/step_definitions/organisation_steps.tstests/cucumber/step_definitions/resolve_config_steps.tstests/cucumber/step_definitions/secret_steps.tstests/cucumber/step_definitions/type_template_steps.tstests/cucumber/step_definitions/variable_steps.tstests/cucumber/step_definitions/workspace_steps.tstests/cucumber/step_definitions_ui/common_steps.tstests/cucumber/step_definitions_ui/config_steps.tstests/cucumber/step_definitions_ui/context_steps.tstests/cucumber/step_definitions_ui/default_config_steps.tstests/cucumber/step_definitions_ui/dimension_steps.tstests/cucumber/step_definitions_ui/experiment_group_steps.tstests/cucumber/step_definitions_ui/experiment_steps.tstests/cucumber/step_definitions_ui/function_steps.tstests/cucumber/step_definitions_ui/organisation_steps.tstests/cucumber/step_definitions_ui/resolve_config_steps.tstests/cucumber/step_definitions_ui/secret_steps.tstests/cucumber/step_definitions_ui/type_template_steps.tstests/cucumber/step_definitions_ui/variable_steps.tstests/cucumber/step_definitions_ui/workspace_steps.tstests/cucumber/support/hooks.tstests/cucumber/support/world.tstests/cucumber/support_ui/hooks.tstests/cucumber/support_ui/world.tstests/cucumber/tsconfig.json
Comment on lines
+60
to
+87
| Given( | ||
| "a context exists with condition {string} equals {string} and override {string} to {string}", | ||
| async function ( | ||
| this: PlaywrightWorld, | ||
| dimName: string, | ||
| dimValue: string, | ||
| configKey: string, | ||
| configValue: string | ||
| ) { | ||
| try { | ||
| const response = await this.client.send( | ||
| new CreateContextCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| request: { | ||
| context: { [dimName]: dimValue }, | ||
| override: { [configKey]: configValue }, | ||
| description: "Cucumber test context", | ||
| change_reason: "Cucumber setup", | ||
| }, | ||
| }) | ||
| ); | ||
| this.contextId = response.id ?? ""; | ||
| this.createdContextIds.push(this.contextId); | ||
| } catch { | ||
| // May already exist | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Recover the existing context ID instead of swallowing the error.
If this setup hits an "already exists" path, this.contextId stays empty and every later step in the scenario uses that empty id. Either make the context unique or look up the existing context and store its id in the catch block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/context_steps.ts` around lines 60 - 87,
The catch block currently swallows errors causing this.contextId to remain empty
when CreateContextCommand fails; modify the error handling in the Given step
that calls CreateContextCommand so that on an "already exists" error you query
for the existing context (e.g., call the appropriate list/get context API using
this.client and the same workspace_id/org_id and matching request
context/override), set this.contextId to the returned id and push it into
this.createdContextIds, otherwise rethrow or log non-duplicate errors;
alternatively ensure the created context is unique by appending a test-specific
suffix to the context data before calling CreateContextCommand so you never hit
the duplicate path.
Comment on lines
+348
to
+366
| When( | ||
| "I perform a bulk operation to create contexts for {string} values {string}", | ||
| async function (this: PlaywrightWorld, dimName: string, values: string) { | ||
| const valueList = values.split(",").map((v) => v.trim()); | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new BulkOperationCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| operations: valueList.map((val) => ({ | ||
| PUT: { | ||
| context: { [dimName]: val }, | ||
| override: { "ctx-config-key": `${val}-bulk` }, | ||
| description: `Bulk context for ${val}`, | ||
| change_reason: "Cucumber bulk test", | ||
| }, | ||
| })), | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Bulk-created contexts are never registered for teardown.
This step can create several contexts, but none of their ids are added to this.createdContextIds. tests/cucumber/support_ui/hooks.ts only deletes tracked ids, so bulk scenarios leave residue that can interfere with later runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/context_steps.ts` around lines 348 - 366,
The bulk-create step handler is not recording created context IDs for teardown;
after sending the BulkOperationCommand (in the When step handler that builds
valueList and sets this.lastResponse) inspect the response from the client.send
call for the created context entries, extract their ids, and append them to
this.createdContextIds so later hooks (tests/cucumber/support_ui/hooks.ts) can
delete them; update the code path around the await this.client.send(...) call to
parse the returned operation results and push each created context id into
this.createdContextIds.
Comment on lines
+406
to
+413
| Then( | ||
| "the response should include the override for {string}", | ||
| function (this: PlaywrightWorld, configKey: string) { | ||
| assert.ok(this.lastResponse, "No response"); | ||
| const override = this.lastResponse.override ?? this.lastResponse.r_override; | ||
| assert.ok(override, "No override in response"); | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Assert the requested override key, not just any override.
configKey is unused, so a response with the wrong override still passes as long as some override exists. This needs to check that the returned override object actually contains the requested key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/context_steps.ts` around lines 406 - 413,
The step currently only asserts that some override object exists but never uses
configKey; update the Then step handler (the function in the Then("the response
should include the override for {string}", ...) closure using PlaywrightWorld
and this.lastResponse) to verify that the override object contains the requested
key: fetch the override as before (this.lastResponse.override ??
this.lastResponse.r_override), assert it exists, then assert
Object.prototype.hasOwnProperty.call(override, configKey) (or that
override[configKey] !== undefined) with a clear error message if missing so the
test fails when the specific configKey is not present.
Comment on lines
+415
to
+422
| Then( | ||
| "the list should contain the created context", | ||
| function (this: PlaywrightWorld) { | ||
| const data = this.lastResponse?.data ?? this.lastResponse; | ||
| assert.ok(Array.isArray(data), "Response is not a list"); | ||
| assert.ok(data.length > 0, "List is empty"); | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
This step does not verify the created context is in the list.
It only checks that the response is a non-empty array. Any existing context makes the assertion pass, even if the newly created or moved context is missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/context_steps.ts` around lines 415 - 422,
The step currently only checks the response is a non-empty array; update it to
assert the created context is present by locating the created context object
(e.g., this.lastCreatedContext or this.createdContext) and checking the list
(this.lastResponse?.data) contains an item with the same unique identifier (id
or name). Specifically, in the "the list should contain the created context"
step, find an element in the array where item.id === this.lastCreatedContext.id
(or item.name === this.lastCreatedContext.name) and assert that such an element
exists with a clear error message.
Comment on lines
+21
to
+27
| // Track group-specific experiment IDs | ||
| let validExpId: string; | ||
| let validExp2Id: string; | ||
| let inProgressExpId: string; | ||
| let conflictingContextExpId: string; | ||
|
|
||
| const groupContext = { os: "ios", clientId: "groupClient" }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/cucumber/step_definitions_ui/experiment_group_steps.ts | head -50Repository: juspay/superposition
Length of output: 2073
🏁 Script executed:
fd -t f "cucumber" -o | grep -E "\.(js|ts|json)$" | head -20Repository: juspay/superposition
Length of output: 180
🏁 Script executed:
fd -t f "world" -o | grep -i playwright | head -20Repository: juspay/superposition
Length of output: 180
🏁 Script executed:
fd -t f "cucumber" | grep -iE "(config|profile)" | head -20Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
find tests -name "*.config.*" -o -name "cucumber.json" -o -name ".cucumber*" 2>/dev/null | head -20Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
cat -n tests/cucumber/support_ui/world.ts | head -100Repository: juspay/superposition
Length of output: 4592
🏁 Script executed:
rg "PlaywrightWorld" tests/cucumber --type ts | head -30Repository: juspay/superposition
Length of output: 3241
🏁 Script executed:
rg "parallel\|workers\|retry" tests/cucumber -A 2 --type ts --type json | head -40Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
cat -n tests/cucumber/step_definitions_ui/experiment_group_steps.ts | tail -300Repository: juspay/superposition
Length of output: 11227
🏁 Script executed:
rg "validExpId|validExp2Id|inProgressExpId|conflictingContextExpId" tests/cucumber/step_definitions_ui/experiment_group_steps.ts -B 2 -A 5Repository: juspay/superposition
Length of output: 3361
🏁 Script executed:
find tests -type f \( -name "*.json" -o -name "*.config.ts" -o -name "*.config.js" -o -name "cucumber.js" \) 2>/dev/nullRepository: juspay/superposition
Length of output: 219
🏁 Script executed:
find . -maxdepth 3 -type f -name "*.json" | xargs grep -l "cucumber\|parallel\|workers" 2>/dev/null | head -10Repository: juspay/superposition
Length of output: 259
🏁 Script executed:
cat -n tests/cucumber/cucumber.jsRepository: juspay/superposition
Length of output: 1033
🏁 Script executed:
cat -n tests/cucumber/package.json | grep -A 20 "cucumber\|scripts"Repository: juspay/superposition
Length of output: 2800
🏁 Script executed:
find tests/cucumber -name "*.feature" | head -5 | xargs head -20Repository: juspay/superposition
Length of output: 4150
🏁 Script executed:
cat -n tests/cucumber/features/experiment_group.featureRepository: juspay/superposition
Length of output: 5210
🏁 Script executed:
rg "workers|parallel|retry|maxInstances" tests/cucumber --type json --type js -A 2Repository: juspay/superposition
Length of output: 507
🏁 Script executed:
ls -la tests/cucumber/support_ui/Repository: juspay/superposition
Length of output: 291
🏁 Script executed:
rg "^let\s+\w+.*:" tests/cucumber/step_definitions_ui/*.ts | head -30Repository: juspay/superposition
Length of output: 422
🏁 Script executed:
cat -n tests/cucumber/support_ui/hooks.ts | head -50Repository: juspay/superposition
Length of output: 2257
🏁 Script executed:
grep -r "^let\s" tests/cucumber/step_definitions_ui/*.ts | grep -v "^[[:space:]]*let" | head -20Repository: juspay/superposition
Length of output: 422
🏁 Script executed:
rg "^let\s+\w+\s*:" tests/cucumber/step_definitions_ui --type tsRepository: juspay/superposition
Length of output: 422
🏁 Script executed:
cat -n tests/cucumber/support_ui/hooks.ts | tail -100Repository: juspay/superposition
Length of output: 2834
🏁 Script executed:
grep -A 10 "^Before\|^After" tests/cucumber/support_ui/hooks.ts | head -40Repository: juspay/superposition
Length of output: 1368
🏁 Script executed:
cat -n tests/cucumber/support_ui/hooks.ts | grep -A 30 "^Before(async"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
cat -n tests/cucumber/support_ui/hooks.ts | grep -A 50 "^After(async"Repository: juspay/superposition
Length of output: 46
🏁 Script executed:
sed -n '155,220p' tests/cucumber/support_ui/hooks.tsRepository: juspay/superposition
Length of output: 1699
Move experiment fixture IDs to PlaywrightWorld instance properties, not module scope.
Module-scoped let bindings are shared across all scenarios within the test run and persist between scenarios. If a scenario fails partway through or if test execution is ever refactored to use parallel workers, one scenario's experiment IDs can pollute another's. Assign these to instance properties of this (the PlaywrightWorld object) instead, following the same pattern already used for createdExperimentIds.
No changes needed to Cucumber configuration; the UI profile is not currently configured for parallel workers or retries, so that concern does not apply.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions_ui/experiment_group_steps.ts` around lines 21
- 27, The module-scoped experiment ID variables (validExpId, validExp2Id,
inProgressExpId, conflictingContextExpId) should be moved off the module scope
and stored as properties on the PlaywrightWorld instance (this) instead—follow
the existing pattern used for createdExperimentIds; update any references in
experiment_group_steps.ts to read/write this.validExpId, this.validExp2Id,
this.inProgressExpId, this.conflictingContextExpId, and initialize them on the
world instance before/when they are assigned so IDs are scenario-scoped and no
longer shared across scenarios or workers.
Comment on lines
+651
to
+657
| Then( | ||
| "the list should contain the created group", | ||
| function (this: SuperpositionWorld) { | ||
| const data = this.lastResponse?.data; | ||
| assert.ok(Array.isArray(data), "Response is not a list"); | ||
| assert.ok(data.length > 0, "List is empty"); | ||
| } |
Contributor
There was a problem hiding this comment.
This step only proves the list is non-empty.
It never checks that the created group is present, so any existing experiment group satisfies the assertion. Please assert against this.experimentGroupId or the created name in lastResponse.data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/experiment_group_steps.ts` around lines 651 -
657, The step definition "the list should contain the created group" currently
only asserts the response is a non-empty array; update the assertion to verify
the actual created group is included by checking this.lastResponse?.data for an
item matching this.experimentGroupId or the created group's name on the
SuperpositionWorld (use this.experimentGroupId if available, otherwise compare
against the stored name), e.g., iterate the array from this.lastResponse.data
and assert some(item => item.id === this.experimentGroupId || item.name ===
this.experimentGroupName) so the step guarantees the specific created group is
present.
Comment on lines
+130
to
+139
| Then( | ||
| "the config value should be either the default or experimental value", | ||
| function (this: SuperpositionWorld) { | ||
| assert.ok(this.lastResponse, "No response"); | ||
| assert.ok(this.lastResponse.config, "No config in response"); | ||
| const value = (this.lastResponse.config as any)[this.configKey]; | ||
| assert.ok( | ||
| value === DEFAULT_VALUE || value === EXPERIMENTAL_VALUE, | ||
| `Expected "${DEFAULT_VALUE}" or "${EXPERIMENTAL_VALUE}", got "${value}"` | ||
| ); |
Contributor
There was a problem hiding this comment.
This step does not actually validate bucketing.
It resolves once at 50% traffic and accepts either branch, so a resolver that always returns the default, always returns the experimental value, or ignores identifier entirely will still pass. Please make this deterministic or assert stickiness across repeated calls for the same identifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/resolve_config_steps.ts` around lines 130 -
139, The step currently only checks that a single resolution is one of
DEFAULT_VALUE or EXPERIMENTAL_VALUE and so doesn't validate bucketing or
stickiness; update the step in the Then(...) handler (the step function using
SuperpositionWorld, this.lastResponse, this.configKey) to call the resolver
multiple times for the same identifier (use the same this.identifier or set one
on the world) and assert that repeated calls return the same value as the first
response (value === DEFAULT_VALUE || value === EXPERIMENTAL_VALUE, and
subsequent responses strictly equal the firstValue), thereby verifying
stickiness; optionally also resolve with a different identifier and assert the
value may differ to ensure bucketing varies by identifier.
Comment on lines
+110
to
+127
| When( | ||
| "I list workspaces filtered by status {string}", | ||
| async function (this: SuperpositionWorld, status: string) { | ||
| try { | ||
| this.lastResponse = await this.client.send( | ||
| new ListWorkspaceCommand({ | ||
| count: 5, | ||
| page: 1, | ||
| org_id: this.orgId, | ||
| }) | ||
| ); | ||
| this.lastError = undefined; | ||
| } catch (e: any) { | ||
| this.lastError = e; | ||
| this.lastResponse = undefined; | ||
| } | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SDK documentation or type definitions for ListWorkspaceCommand parameters
rg -n "ListWorkspaceCommand" --type=ts -C5 | head -100Repository: juspay/superposition
Length of output: 5572
Status filter parameter is ignored.
The step definition accepts a status parameter but does not pass it to ListWorkspaceCommand, causing the filter to be bypassed regardless of the intended test behavior.
🐛 Proposed fix to use the status filter
When(
"I list workspaces filtered by status {string}",
async function (this: SuperpositionWorld, status: string) {
try {
this.lastResponse = await this.client.send(
new ListWorkspaceCommand({
count: 5,
page: 1,
org_id: this.orgId,
+ status: status as WorkspaceStatus,
})
);
this.lastError = undefined;
} catch (e: any) {
this.lastError = e;
this.lastResponse = undefined;
}
}
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/step_definitions/workspace_steps.ts` around lines 110 - 127,
The step definition for "I list workspaces filtered by status {string}"
currently ignores the status parameter; update the async step handler so it
passes the status into the ListWorkspaceCommand payload (e.g., add a status
field to the object passed to new ListWorkspaceCommand) within the function
handling the step in workspace_steps.ts so the ListWorkspaceCommand receives and
applies the filter.
Comment on lines
+22
to
+29
| const TEST_ORG_NAME = "cucumberorg"; | ||
| const TEST_WORKSPACE = "cucumberws"; | ||
|
|
||
| let browser: Browser; | ||
|
|
||
| // Shared state across scenarios (org/workspace persist for the run) | ||
| let sharedOrgId: string = ""; | ||
| let sharedWorkspaceId: string = ""; |
Contributor
There was a problem hiding this comment.
The suite is reusing global fixtures instead of creating isolated ones.
BeforeAll binds every scenario to the first matching cucumberorg* / cucumberws, so leftovers from failed runs or concurrent CI jobs change setup behavior and list results. Using a run-scoped org/workspace name and deleting it in AfterAll would keep these tests self-contained.
Also applies to: 31-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/support_ui/hooks.ts` around lines 22 - 29, The tests currently
reuse global fixtures (TEST_ORG_NAME, TEST_WORKSPACE, sharedOrgId,
sharedWorkspaceId) and bind scenarios via BeforeAll to existing org/workspace,
causing cross-run contamination; change TEST_ORG_NAME and TEST_WORKSPACE to
generate unique, run-scoped names (e.g., append timestamp/UUID) and create the
org/workspace in BeforeAll (using the same setup code that currently looks up
existing cucumberorg*/cucumberws*), store their IDs in sharedOrgId and
sharedWorkspaceId, and delete those created resources in AfterAll (ensuring
deletion in a finally handler so cleanup runs on failures); update any
references in hooks.ts and related setup helpers to use the new generator
functions and the sharedId variables so each test run is self-contained.
Comment on lines
+182
to
+211
| // Cleanup experiment groups | ||
| if (this.experimentGroupId) { | ||
| try { | ||
| await this.client.send( | ||
| new DeleteExperimentGroupCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| id: this.experimentGroupId, | ||
| }) | ||
| ); | ||
| } catch { | ||
| // May have members or already deleted | ||
| } | ||
| } | ||
|
|
||
| // Cleanup experiments | ||
| for (const id of this.createdExperimentIds) { | ||
| try { | ||
| await this.client.send( | ||
| new DiscardExperimentCommand({ | ||
| workspace_id: this.workspaceId, | ||
| org_id: this.orgId, | ||
| id, | ||
| change_reason: "Cucumber cleanup", | ||
| }) | ||
| ); | ||
| } catch { | ||
| // Already discarded or concluded | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Delete experiment groups after member experiments are cleaned up.
The group delete happens first and its failure is swallowed. If the backend rejects deleting groups with members, the later discard calls still run but there is never a second delete attempt, so the group leaks into the shared workspace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cucumber/support_ui/hooks.ts` around lines 182 - 211, The cleanup
currently calls DeleteExperimentGroupCommand before discarding experiments which
can leak groups if deletion is rejected due to members; reorder the logic in the
hook so you first iterate this.createdExperimentIds and await
this.client.send(DiscardExperimentCommand(...)) for each id, then after that, if
this.experimentGroupId is set call
this.client.send(DeleteExperimentGroupCommand(...)) to remove the group; keep
catch blocks but ensure the group delete happens after the discard loop (or
retry the delete once inside the existing catch) so groups are removed once
members are cleaned up.
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.
Problem
The Superposition project lacks comprehensive behavior-driven development (BDD) tests. Manual testing is time-consuming and error-prone, and there's no automated way to verify API functionality across all features.
Solution
Added a complete Cucumber/Gherkin BDD test suite that provides comprehensive coverage of the Superposition API. The test suite includes:
Test Coverage:
Architecture:
SuperpositionWorldclass maintains test state across scenariosTest Infrastructure:
Environment variable changes
SUPERPOSITION_BASE_URL: API endpoint (defaults tohttp://127.0.0.1:8080)SUPERPOSITION_TOKEN: Authentication token (defaults tosome-token)Pre-deployment activity
None required. Tests are self-contained and create their own test org/workspace.
Post-deployment activity
npm testfrom thetests/cucumberdirectorytests/cucumber/reports/cucumber-report.htmlAPI changes
N/A - No API changes. Tests exercise existing API endpoints.
Possible Issues in the future
https://claude.ai/code/session_01GBgZKZt7NTb8rZhZW2Q7iA
Summary by CodeRabbit
New Features
Tests
Chores