Conversation
…chore/componentise-feature-override-row # Conflicts: # frontend/web/components/feature-override/FeatureOverrideRow.tsx
…ponentise-feature-filters
…ponentise-feature-filters
…ponentise-feature-filters
…eature-state-view
# Conflicts: # frontend/README.md # frontend/package-lock.json
# Conflicts: # frontend/e2e/helpers.cafe.ts # frontend/e2e/tests/invite-test.ts # frontend/package-lock.json
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| module.exports = [ | ||
|
|
||
| new webpack.DefinePlugin({ | ||
| E2E: process.env.E2E, |
There was a problem hiding this comment.
Removing E2E from DefinePlugin causes production ReferenceError
High Severity
Removing E2E: process.env.E2E from webpack's DefinePlugin breaks production. ValueEditor.js and organisation-store.js still reference bare E2E as a global variable. Previously, DefinePlugin replaced every E2E reference with the literal undefined at build time, which is safe. Now, E2E remains as an undeclared variable in the compiled bundle, causing a ReferenceError at runtime in production (where window.E2E is never set). The window.E2E = true injection in test-setup.ts only runs during Playwright tests, not in production.
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
File read before existence check causes crash
Low Severity
failed.json is read eagerly at module scope (line 8) with fs.readFileSync, but the fs.existsSync guard is at line 82 — after the read. If the file doesn't exist (e.g., tests crashed before the reporter could write it), the script throws on line 8 and the existence check is never reached. The guard needs to come before the read.
Additional Locations (1)
| API_IMAGE: ${{ inputs.api-image }} | ||
| E2E_IMAGE: ${{ inputs.e2e-image }} | ||
| E2E_CONCURRENCY: ${{ inputs.concurrency }} | ||
| E2E_RETRIES: 2 |
There was a problem hiding this comment.
E2E_RETRIES appears misindented outside env mapping
Medium Severity
E2E_RETRIES: 2 appears to be indented at a different level than the other environment variables under env:. The other env vars (opts, API_IMAGE, E2E_CONCURRENCY, SLACK_TOKEN) are consistently indented 2 spaces deeper than env:, but E2E_RETRIES appears to be at the same level as env: itself. This would place it outside the env mapping, meaning it won't be set as an environment variable — or it could cause a YAML parse error entirely.
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
Slack reporter crashes before file existence check runs
High Severity
The fs.readFileSync on line 8 runs at module load time, before the fs.existsSync guard on line 82 has a chance to execute. When failed.json doesn't exist (e.g., if global setup fails before any tests run), the script throws an unhandled ENOENT error and crashes instead of gracefully exiting with the "No failed.json found" message.
Additional Locations (1)
| API_IMAGE: ${{ inputs.api-image }} | ||
| E2E_IMAGE: ${{ inputs.e2e-image }} | ||
| E2E_CONCURRENCY: ${{ inputs.concurrency }} | ||
| E2E_RETRIES: 2 |
There was a problem hiding this comment.
E2E_RETRIES not forwarded into Docker container
Medium Severity
The CI workflow sets E2E_RETRIES: 2 as a step-level environment variable, but the Makefile only passes E2E_CONCURRENCY to the Docker container via cross-env. Since the docker-compose environment section for the frontend service also doesn't list E2E_RETRIES, this value never reaches run-with-retry.ts inside the container, which defaults to 1 retry instead of the intended 2.
Additional Locations (1)
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
File read executes before existence check, crashes on missing file
Medium Severity
The fs.readFileSync on failedJsonPath at module level (line 8) executes before the fs.existsSync guard on line 82. If failed.json doesn't exist (e.g., tests failed during global setup before any test ran), the script throws an unhandled ENOENT error and crashes instead of gracefully exiting. The existence check on line 82 is effectively dead code and never runs in the missing-file scenario. The check and early exit need to come before the file read.
Additional Locations (1)
| "test": "npm run test:bundle && cross-env NODE_ENV=production E2E=true ts-node -T ./e2e/index.cafe", | ||
| "test:staging": "npm run test:bundle:staging && cross-env NODE_ENV=production E2E=true ENV=staging ts-node -T ./e2e/index.cafe", | ||
| "test": "npx -y tsx e2e/run-with-retry.ts", | ||
| "test:run": "cross-env npm run bundle && npx playwright test", |
There was a problem hiding this comment.
Invalid cross-env usage without environment variable
High Severity
The test:run script uses cross-env without specifying any environment variables to set. The syntax cross-env npm run bundle is invalid because cross-env requires at least one VAR=value pair before the command (e.g., cross-env E2E=true npm run bundle). This will cause the script to fail or behave unexpectedly when executed directly.
frontend/e2e/tests/roles-test.pw.ts
Outdated
| log('Add project permissions to the Role') | ||
| await click(byId(`role-0`)) | ||
| await click(byId('permissions-tab')) | ||
| await click(byId('permissions-tab')) |
There was a problem hiding this comment.
Duplicate click on permissions tab in role test
Medium Severity
The permissions-tab element is clicked twice in succession with identical calls to click(byId('permissions-tab')). This appears to be a copy-paste error and is redundant. While unlikely to cause test failures, it adds unnecessary delay and could cause issues if the UI responds unexpectedly to double-clicks.
| await login(E2E_NON_ADMIN_USER_WITH_ENV_PERMISSIONS, PASSWORD) | ||
| await gotoProject(PROJECT_NAME) | ||
| await waitForElementVisible(byId('switch-environment-production')) | ||
| await waitForElementVisible(byId('switch-environment-production')) |
There was a problem hiding this comment.
Duplicate wait for same element in environment test
Low Severity
The switch-environment-production element is waited for twice in consecutive lines using identical waitForElementVisible(byId('switch-environment-production')) calls. This is redundant code that serves no purpose and should be reduced to a single call.
| // Skip cleanup on retries to preserve failed.json and test artifacts | ||
| if (isRetry) { | ||
| playwrightCmd.push('E2E_SKIP_CLEANUP=1'); | ||
| } |
There was a problem hiding this comment.
Retry logic broken by missing artifact preservation
Medium Severity
The retry mechanism sets E2E_SKIP_CLEANUP=1 to preserve test artifacts but never checks this variable. Playwright cleans the outputDir (including .last-run.json) at the start of each test run by default. This breaks the --last-failed flag at line 151, which depends on .last-run.json to identify failed tests. On retry, all tests will re-run instead of just the failed ones, defeating the purpose of the retry optimization and wasting execution time.
Additional Locations (1)
# Conflicts: # frontend/e2e/tests/segment-test.ts # frontend/package-lock.json
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
File read crashes before existence check is reached
High Severity
fs.readFileSync(failedJsonPath, 'utf-8') on line 8 executes unconditionally at module load, before the fs.existsSync guard on line 82 is ever reached. If failed.json doesn't exist, the script crashes with an unhandled ENOENT error. Since this script only runs on CI test failures (if: failure()), the file may well be absent — for example, when global setup fails before any test executes. The existence check and early process.exit(0) are dead code in the missing-file scenario. Moving the file read after the existence check (or into main()) would fix this.
Additional Locations (1)
| API_IMAGE: ${{ inputs.api-image }} | ||
| E2E_IMAGE: ${{ inputs.e2e-image }} | ||
| E2E_CONCURRENCY: ${{ inputs.concurrency }} | ||
| E2E_RETRIES: 2 |
There was a problem hiding this comment.
E2E_RETRIES not forwarded to Docker container in CI
Medium Severity
E2E_RETRIES: 2 is set in the CI workflow step env but never reaches the Docker container. The Makefile explicitly forwards E2E_CONCURRENCY to the container via cross-env in the sh -c command, but E2E_RETRIES is not included. The docker-compose-e2e-tests.yml frontend service environment section also doesn't list it. So run-with-retry.ts inside the container always uses the default value of 1, making the CI setting ineffective.
Additional Locations (1)
The CI workflow sets E2E_RETRIES=2 but the Makefile only forwarded E2E_CONCURRENCY to the container, so retries defaulted to 1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
File read crashes before existence check executes
Medium Severity
The top-level fs.readFileSync(failedJsonPath, 'utf-8') on line 8 executes at module load time, before the fs.existsSync(failedJsonPath) guard on line 82. If failed.json doesn't exist (e.g., global setup failure prevents any tests from running), the script crashes with an unhandled error instead of gracefully exiting. The existence check needs to be moved before the file read.
Additional Locations (1)
frontend/Makefile
Outdated
| || (docker compose logs flagsmith-api; exit 1) | ||
| @echo "Running E2E tests..." | ||
| @docker compose run --name e2e-test-run frontend \ | ||
| sh -c 'npx cross-env E2E_CONCURRENCY=${E2E_CONCURRENCY} npm run test -- $(opts)' \ |
There was a problem hiding this comment.
E2E_RETRIES not forwarded to Docker container
Medium Severity
The Makefile test target passes E2E_CONCURRENCY via cross-env to the Docker container but omits E2E_RETRIES. The CI workflow sets E2E_RETRIES: 2 as a step-level env var, but since the Makefile doesn't include it in the cross-env command and it's not in docker-compose-e2e-tests.yml, the test runner inside the container defaults to 1 retry instead of the intended 2.
Additional Locations (1)
|
Great work on the Playwright migration @kyle-ssg ! The DX improvements (trace viewer, DOM snapshots, interactive UI mode) are huge. I tested extensively using both Test ResultsUsing Using Using Observations1. Enterprise tests fail against OSS API Two tests consistently fail locally:
Root cause: 2. Concurrency mismatch
3. Found instances in CI handles OSS/Enterprise correctlyLooking at
Can't build private-cloud locallyTried changing to Questions
None of these are blockers. The PR is really good and ready to merge! I just wanted to raise these observations for awareness and to clarify the expected local development workflow. |
…ywright # Conflicts: # frontend/e2e/tests/project-test.pw.ts
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const CHANNEL_ID = 'C0102JZRG3G'; // infra_tests channel ID | ||
| const failedJsonPath = path.join(__dirname, 'test-results', 'failed.json'); | ||
| const failedData = JSON.parse(fs.readFileSync(failedJsonPath, 'utf-8')); | ||
| const failedCount = failedData.failedTests?.length || 0; |
There was a problem hiding this comment.
File read executes before existence check crashes script
Medium Severity
The failed.json file is read and parsed at module top-level (lines 7–9) with fs.readFileSync and JSON.parse, but the fs.existsSync guard that checks whether the file exists doesn't run until line 82. If failed.json is absent (e.g., global setup fails before any test runs), the script throws an uncaught ENOENT error at module load time, before reaching the existence check or the main().catch() handler. The read and existence check need to be reordered so the guard runs first.
Additional Locations (1)
3 participants
docs/if required so people know about the feature.Changes
Playwright Test Improvements
Faster Tests
Better Local Development
Added Claude Commands
/e2e- Run all tests (OSS + Enterprise)/e2e-oss- Run OSS tests, auto-fix failures, re-run until passing/e2e-ee- Run Enterprise tests, auto-fix failures, re-run until passing/e2e-create- Create a new test following existing patternsNote: You can have the tests repeat to guarantee no flakiness
HTML Reports
e.g. this is a downloaded report, we can access reports of failures as well as videos / interactive traces.
CI/CD Features
GitHub Integration
How did you test this code?