feat: Playwright test framework - API & UI automation - #1678
Open
pranavexe wants to merge 7 commits into
Open
Conversation
- Added global setup - Restructured specs folder - Introduced auth actions structure - Added support utilities - Updated playwright config - Updated gitignore for Playwright artifacts
- Add ApiClient, TokenManager, RequestWrapper - Add ResponseValidator, SchemaValidator - Add NetworkActions for request mocking - Add auth.api.spec.ts - first backend API tests - Fix playwright.config.ts with proper setup - Add support files: api.routes, application.routes, translations
…ilityId to state file
- API/UI test separation - no browser for API tests - Global setup: upload, scan, state management - Feature flags: env-aware test skipping - Dynamic scan: 4 flows with polling utils - Constants: exact status codes from backend - Multi-env support: npm run playwright:qa/prod - Response and schema validation - Allure reporting
WalkthroughThis pull request introduces a comprehensive Playwright test framework for API and UI testing, including authentication flows, global setup orchestration with state management, multiple API test suites covering endpoints, configuration setup with environment variables, and supporting utilities for request/response validation, route resolution, and test state management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
Deploying irenestaging with
|
| Latest commit: |
e7e9639
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://60ebcb78.irenestaging.pages.dev |
| Branch Preview URL: | https://playwright-qa-setup.irenestaging.pages.dev |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
1-256:⚠️ Potential issue | 🔴 CriticalCritical:
package-lock.jsonis out of sync — pipeline is failing.The CI pipeline shows
npm cifailures due to missing dependencies in the lock file. Runnpm installto regeneratepackage-lock.jsonand commit the updated lock file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 1 - 256, The package-lock.json is out of sync causing CI failure on npm ci; run npm install locally to regenerate package-lock.json (ensuring Node/npm versions match engines/volta constraints), verify no unexpected changes, then add and commit the updated package-lock.json so the pipeline can pass; reference package.json (engines/volta) to ensure compatible Node/npm when reproducing.
🟡 Minor comments (8)
playwright/support/translations.ts-10-21 (1)
10-21:⚠️ Potential issue | 🟡 MinorFallback returns partial key segment instead of full key path.
When a nested property is missing, the
reducereturns only the last unmatched segment (c) rather than the original full key. This can make debugging failed translation lookups difficult since you lose context about which key was attempted.Suggested improvement
function getMessageFromTranslations( key: NestedKeyOf<typeof EN_TRANSLATIONS> ): string { return key .split('.') .reduce((p: Record<string, string | object> | string, c) => { if (typeof p === 'object' && c in p) { return p[c] as string; } - return c; + return key; // Return full key for easier debugging when lookup fails }, EN_TRANSLATIONS) as string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/support/translations.ts` around lines 10 - 21, getMessageFromTranslations currently returns only the last unmatched segment (variable `c`) when a nested translation is missing, losing context; change it to return the original full key string instead. Locate the function getMessageFromTranslations and EN_TRANSLATIONS, keep the reduce traversal but ensure the final result is validated: if the reduce doesn't resolve to a string (or the path is missing), return the original `key` (the full dotted path) as the fallback rather than `c`; alternatively propagate a sentinel object through reduce and then map that to `key` before returning.playwright/support/utils.ts-86-90 (1)
86-90:⚠️ Potential issue | 🟡 MinorReturn type mismatch: function can return
undefined.The return type declares
string, but accessingtypeTextMap[vulnType]with an unrecognized code (e.g.,5,0) returnsundefined. The JSDoc correctly documents this, but the TypeScript signature is inaccurate.Suggested fix
-export function getVulnerabilityTypeText(vulnType: number): string { +export function getVulnerabilityTypeText(vulnType: number): string | undefined { const typeTextMap = { 1: 'static', 2: 'dynamic', 3: 'manual', 4: 'api' }; return typeTextMap[vulnType as keyof typeof typeTextMap]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/support/utils.ts` around lines 86 - 90, The function getVulnerabilityTypeText currently declares a string return but may return undefined when vulnType is not one of the keys in typeTextMap; update the implementation to either (A) widen the signature to return string | undefined and keep the current lookup, or (B) validate vulnType against the keys in typeTextMap (e.g., via Object.prototype.hasOwnProperty or a map lookup) and return a fallback like 'unknown' or throw a controlled error — apply the change to the getVulnerabilityTypeText function and adjust any callers/tests that rely on a guaranteed string accordingly.playwright/utils/schema.validator.ts-46-51 (1)
46-51:⚠️ Potential issue | 🟡 Minor
nullvalues incorrectly passobjecttype validation.In JavaScript,
typeof null === 'object'. If a required field isnullbut the schema expectsobject, the validation will incorrectly pass. Consider adding an explicit null check.Suggested fix
} else { + if (value === null) { + throw new Error( + `Schema violation: '${field}' should be ${rules.type} but got null` + ); + } expect( typeof value, `Schema violation: '${field}' should be ${rules.type} but got ${typeof value}` ).toBe(rules.type); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/utils/schema.validator.ts` around lines 46 - 51, The validator currently uses expect(typeof value ...) to check types which treats null as 'object'; update the check in the branch that uses value, field and rules.type so that it first explicitly fails when value === null for expected object types (i.e., if rules.type === 'object' and value === null, call expect to fail with the same schema violation message), otherwise continue with the typeof check; modify the block around the expect(...) using value/field/rules.type to include this null guard so null does not pass object validation.playwright/specs/api/report.api.spec.ts-160-163 (1)
160-163:⚠️ Potential issue | 🟡 MinorTypo: "SHEILD" should be "SHIELD".
✏️ Proposed fix
test.skip( !state.features.privacy, - 'PRIVACY SHEILD NOT ENABLED ON THIS ENVIRONMENT' + 'PRIVACY SHIELD NOT ENABLED ON THIS ENVIRONMENT' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/specs/api/report.api.spec.ts` around lines 160 - 163, The test skip message contains a typo: replace the string 'PRIVACY SHEILD NOT ENABLED ON THIS ENVIRONMENT' used in the test.skip call with the corrected 'PRIVACY SHIELD NOT ENABLED ON THIS ENVIRONMENT' (locate the test.skip invocation in report.api.spec.ts and update the message literal).playwright/support/api.routes.ts-145-148 (1)
145-148:⚠️ Potential issue | 🟡 MinorTypo in route key:
sbReortCyclonedxshould besbReportCyclonedx.The key has a typo ("Reort" vs "Report") while the alias is correct. This inconsistency could cause confusion.
✏️ Proposed fix
- sbReortCyclonedx: { + sbReportCyclonedx: { route: '/api/v2/sb_reports/*/cyclonedx_json_file/download_url', alias: 'sbReportCyclonedx', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/support/api.routes.ts` around lines 145 - 148, Rename the mistyped route key sbReortCyclonedx to sbReportCyclonedx so it matches the alias sbReportCyclonedx and avoids confusion; update the object entry that contains route: '/api/v2/sb_reports/*/cyclonedx_json_file/download_url' and alias: 'sbReportCyclonedx' to use the corrected key name sbReportCyclonedx.playwright/specs/api/report.api.spec.ts-18-20 (1)
18-20:⚠️ Potential issue | 🟡 MinorInconsistent cleanup: missing
TokenManager.clearTokens()in afterAll.Other spec files (e.g.,
auth.api.spec.ts,projects.api.spec.ts) callTokenManager.clearTokens()inafterAll. This file omits it, which may cause token leakage between test runs.🔧 Proposed fix
test.afterAll(async () => { + TokenManager.clearTokens(); await wrapper.dispose(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/specs/api/report.api.spec.ts` around lines 18 - 20, The afterAll cleanup in this spec only calls wrapper.dispose() and omits clearing test tokens; update the test.afterAll block to also call TokenManager.clearTokens() after (or before) awaiting wrapper.dispose() so tokens are removed between runs—modify the test.afterAll in report.api.spec.ts to invoke TokenManager.clearTokens() alongside the existing wrapper.dispose() call.playwright/specs/api/auth.api.spec.ts-118-149 (1)
118-149:⚠️ Potential issue | 🟡 MinorDispose the extra
RequestWrapper.This test creates a second request context at Line 118 and never releases it. Reuse the suite wrapper or wrap this block in
try/finallyso a failing assertion does not leak request contexts across the worker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/specs/api/auth.api.spec.ts` around lines 118 - 149, This test instantiates a new RequestWrapper (testWrapper) and never releases its request context; either remove the new RequestWrapper and reuse the suite-level wrapper (e.g., use the existing requestWrapper/requestSuiteWrapper instead of new RequestWrapper()) or wrap the block that uses testWrapper in try/finally and call the wrapper's teardown method in finally (e.g., await testWrapper.dispose()/close()/teardown()) to ensure the request context is always released even on assertion failure; update references to RequestWrapper.init and RequestWrapper.get accordingly when reusing the suite wrapper.playwright/global.setup.ts-186-191 (1)
186-191:⚠️ Potential issue | 🟡 MinorFail explicitly when no risky analysis exists.
If
analysisBody.results.find(...)returnsundefined, dereferencingriskyAnalysis.idthrows a generic TypeError and hides the real setup problem. Guard the lookup and raise a descriptive setup error instead.🛠️ Possible fix
const riskyAnalysis = analysisBody.results.find( (a: Record<string, unknown>) => (a.computed_risk as number) > 0 ); + if (!riskyAnalysis) { + throw new Error('[Setup] No analysis with computed_risk > 0 found'); + } const analysisId = riskyAnalysis.id as number; const vulnerabilityId = riskyAnalysis.vulnerability as number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/global.setup.ts` around lines 186 - 191, The lookup using analysisBody.results.find(...) can return undefined, so guard the result (riskyAnalysis) before dereferencing; if it's undefined throw a descriptive Error (e.g., "No risky analysis found in analysisBody.results") during setup instead of letting a TypeError surface. Update the block around the find call to check riskyAnalysis and then set analysisId and vulnerabilityId only after the check, referencing the riskyAnalysis variable and the analysisId and vulnerabilityId assignments in your change.
🧹 Nitpick comments (10)
.gitignore (1)
52-61: Redundantnode_modules/entry.Line 53 duplicates the existing entry at line 9 (
/node_modules/). The leading/anchors to the repo root, while the pattern without it matches at any depth. Since the rootnode_modules/is already covered, consider removing this duplicate or clarifying if you intend to ignore nestednode_modules/directories (though those are typically already inside the ignored root).Suggested fix
# Playwright -node_modules/ /test-results/ /playwright-report/ /blob-report/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 52 - 61, The .gitignore contains a duplicate node_modules entry: remove the redundant "node_modules/" line (the unanchored pattern) since "/node_modules/" at the repo root already covers it; if you intentionally need to ignore nested node_modules directories, replace or clarify by keeping the unanchored "node_modules/" and removing the anchored "/node_modules/" instead—update the .gitignore accordingly around the existing node_modules entries.playwright/utils/response.validator.ts (1)
30-31: Consider handling non-JSON response bodies gracefully.
response.json()will throw if the server returns non-JSON content (e.g., HTML error pages, empty body on 204/500). This could produce confusing error messages in test failures. Consider wrapping with a try-catch to provide a clearer error message.Suggested improvement
// Parse response body - const body = await response.json(); + let body: Record<string, unknown>; + try { + body = await response.json(); + } catch { + throw new Error( + `Failed to parse JSON from ${response.url()} (status: ${response.status()})` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/utils/response.validator.ts` around lines 30 - 31, The call to response.json() in response.validator.ts can throw for non-JSON or empty responses; wrap the await response.json() in a try-catch inside the validator (around the expression using response.json()) and handle failures by either attempting response.text() as a fallback or setting body to null/undefined, then include a clear error message that includes response.status and response.url (or the response object) when rethrowing or asserting so test failures show that the body was non-JSON rather than crashing with an unhelpful parse error.playwright/Actions/auth/loginActions.ts (1)
14-16: Inconsistent localization: "Next" button uses hardcoded text.The username and password inputs use
pwTranslate()for localized placeholders, but the "Next" button uses a hardcoded English string. This could cause test failures in non-English locales.Suggested fix
- const nextButton = this.page.getByRole('button', { name: 'Next' }); + const nextButton = this.page.getByRole('button', { name: pwTranslate('next') });The translation key
'next'already exists in your translation files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/Actions/auth/loginActions.ts` around lines 14 - 16, Replace the hardcoded "Next" label with the localized translation: change the getByRole call that assigns nextButton (page.getByRole('button', { name: 'Next' })) to use pwTranslate('next') so the button lookup uses the translation key; keep the rest of the flow (expect(nextButton).toBeEnabled() and nextButton.click()) unchanged and ensure pwTranslate is imported/available in loginActions.ts.playwright.config.ts (2)
1-46: Remove commented-out dead code.This 46-line commented block is obsolete configuration that adds noise. Delete it to keep the config clean.
🧹 Proposed fix
-// import { defineConfig } from '@playwright/test'; -// import dotenv from 'dotenv'; -// import path from 'path'; - -// const env = process.env.TEST_ENV || 'qa' || 'prod'; - -// dotenv.config({ -// path: path.resolve(__dirname, `.env.${env}`), -// }); - -// export default defineConfig({ -// testDir: './playwright/specs', -// fullyParallel: true, -// retries: process.env.CI ? 1 : 1, -// workers: process.env.CI ? 4 : 2, -// globalSetup: './playwright/global.setup.ts', -// reporter: [ -// ['html', { open: 'never' }], -// ['line'], -// [ -// 'allure-playwright', -// { -// detail: true, -// outputFolder: 'allure-results', -// suiteTitle: true, -// environmentInfo: { -// Environment: process.env.ENVIRONMENT || env.toUpperCase(), -// BaseURL: process.env.BASE_URL, -// Platform: process.platform, -// }, -// }, -// ], -// ], - -// use: { -// baseURL: process.env.BASE_URL, -// viewport: { width: 1450, height: 1650 }, -// testIdAttribute: 'data-test-cy', -// // storageState: '.auth/user.json', -// trace: 'on-first-retry', -// screenshot: 'only-on-failure', -// video: 'retain-on-failure', -// actionTimeout: 10000, -// navigationTimeout: 60000, -// }, -// }); import { defineConfig } from '@playwright/test';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` around lines 1 - 46, Remove the large commented-out Playwright config block (the 46-line section containing imports for defineConfig, dotenv, path and the commented env/config export) to eliminate dead code; specifically delete the commented lines that reference defineConfig, dotenv.config, the env variable, the export default defineConfig({...}) block and its nested reporter/use settings so the file contains only active configuration or concise alternative config.
60-60: Redundant ternary — both branches return1.The condition
process.env.CI ? 1 : 1always evaluates to1. Simplify to just1, or differentiate CI vs local retries.✏️ Proposed fix
- retries: process.env.CI ? 1 : 1, + retries: 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` at line 60, The ternary expression for the Playwright config retries is redundant: replace the `retries: process.env.CI ? 1 : 1` entry in the Playwright configuration with a single value (e.g., `retries: 1`) or implement distinct values for CI vs local runs if intended; update the retries property in the Playwright config object (the line containing `retries: process.env.CI ? 1 : 1`) accordingly so the branch is no longer redundant.playwright/specs/api/scan.api.spec.ts (1)
81-85: Unnecessary dynamic import —expectis already imported at line 1.The static
import { test } from '@playwright/test'at line 1 can includeexpect. The dynamic import here is redundant overhead.🔧 Proposed fix
Update the static import at the top of the file:
-import { test } from '@playwright/test'; +import { test, expect } from '@playwright/test';Then simplify the step:
await allure.step('Verify static scan is done', async () => { - const { expect } = await import('@playwright/test'); expect(body.static_scan_progress as number).toBe(100); expect(body.is_static_done as boolean).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/specs/api/scan.api.spec.ts` around lines 81 - 85, Remove the unnecessary dynamic import of expect inside the allure.step: add expect to the existing static import from '@playwright/test' at the top of the file (where test is imported), then delete the dynamic "const { expect } = await import('@playwright/test');" line and use the top-level expect in the allure.step assertions (the block referencing body.static_scan_progress and body.is_static_done remains unchanged).playwright/specs/api/report.api.spec.ts (1)
153-158: Redundant wrapper re-initialization in nested describe.The outer
beforeAllalready initializeswrapper. This defensive check is unnecessary if the nested describe runs after the parent'sbeforeAll. Additionally, the nested describe lacks a correspondingafterAllto dispose this second wrapper if it's created.🧹 Proposed simplification
test.describe('Privacy Report', () => { - test.beforeAll(async () => { - if (!wrapper) { - wrapper = new RequestWrapper(); - await wrapper.init(); - } - }); - test.skip(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/specs/api/report.api.spec.ts` around lines 153 - 158, The nested describe is defensively re-initializing the RequestWrapper even though the outer test.beforeAll already sets up wrapper; remove the redundant initialization block inside the nested describe (the beforeAll that constructs and calls wrapper.init()) so the tests reuse the outer wrapper, and if you decide to keep a nested setup, add a matching afterAll that calls wrapper.dispose() (or the appropriate teardown method on RequestWrapper) to avoid leaking resources; reference the symbols wrapper, RequestWrapper, beforeAll, and afterAll when making the change.playwright/support/api.routes.ts (1)
307-316: Silently removing unmatched wildcards may mask bugs.Line 315 removes any remaining
*characters after parameter substitution. If a caller provides fewer params than wildcards, the function silently produces an invalid URL instead of throwing. Consider logging a warning or throwing for mismatched param counts.🛡️ Proposed enhancement for better debugging
export function resolveRoute( route: string, ...params: (string | number)[] ): string { let resolved = route; for (const param of params) { resolved = resolved.replace('*', String(param)); } - return resolved.replace(/\*/g, ''); // remove any remaining wildcards + const remaining = (resolved.match(/\*/g) || []).length; + if (remaining > 0) { + console.warn(`[resolveRoute] ${remaining} unresolved wildcard(s) in: ${resolved}`); + } + return resolved.replace(/\*/g, ''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/support/api.routes.ts` around lines 307 - 316, The resolveRoute helper silently strips unmatched '*' wildcards which can mask caller bugs; update resolveRoute to validate that the number of provided params matches the number of wildcards in the route (e.g., count '*' in the incoming route) before doing replacements, and if they don't match either throw a descriptive Error or at minimum log a warning via your logger; keep the existing replacement logic (replacing '*' with String(param) in the loop over params) but remove the final unconditional replace(/\*/g,'') and instead fail fast when leftover wildcards would remain so callers get an immediate, debuggable failure.playwright/support/test-state.ts (1)
27-28: Add error handling for missing state file.The synchronous
fs.readFileSyncat module load time will throw an unhandled exception if the state file doesn't exist (e.g., if global setup failed or didn't run). Consider wrapping with a try-catch and a descriptive error message.🛡️ Proposed fix
-const state: TestState = JSON.parse(fs.readFileSync(statePath, 'utf-8')); +let state: TestState; +try { + state = JSON.parse(fs.readFileSync(statePath, 'utf-8')); +} catch (err) { + throw new Error( + `Failed to load test state from ${statePath}. ` + + `Ensure global setup has run. Original error: ${err}` + ); +} export default state;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/support/test-state.ts` around lines 27 - 28, Wrap the module-level file read/parse that initializes the `state: TestState` from `statePath` (the `fs.readFileSync` + `JSON.parse`) in a try-catch; on error (especially ENOENT) throw a new Error with a clear message that the state file at `statePath` is missing or failed to parse and advise running global setup, and include the original error details for debugging; ensure `state` is still exported as the default after successful parse.playwright/Actions/api/request.wrapper.ts (1)
5-9: Unused field:requiresAuthis declared but never read.The
requiresAuthoption is defined in the interface but no method uses it. Either remove it or implement the conditional auth logic.🧹 Proposed fix — remove unused field
export interface RequestOptions { endpoint: string; body?: object; - requiresAuth?: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/Actions/api/request.wrapper.ts` around lines 5 - 9, The RequestOptions interface declares requiresAuth but it's never used; remove the requiresAuth property from RequestOptions declared in request.wrapper.ts and update any call sites that pass requiresAuth to stop supplying it (or remove the property from object literals) so the type and usages stay consistent; if you prefer to keep the option instead, implement conditional auth handling inside the module's request-sending function (e.g., add an Authorization header when RequestOptions.requiresAuth is true) and ensure RequestOptions is read where requests are constructed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@auth.json`:
- Around line 8-9: The committed Playwright storage state contains live secrets
under the "ember_simple_auth-session" JSON (keys "token", "b64token",
"user_id"); remove these values from the committed auth JSON, rotate/revoke the
exposed token immediately, and purge the secret from repo history (use git
filter-repo or BFG). Add the auth-state filename(s) to .gitignore and modify
your test bootstrap/globalSetup routine (e.g., the globalSetup function that
creates storage state) to generate the Playwright storage state at runtime
instead of committing it. Ensure the sanitized committed file contains no
credentials and that runtime-generated state is used in CI/local runs.
- Line 5: The persisted auth state currently hardcodes the "origin" value to a
QA host; update the setup so the "origin" field is not committed with an
environment-specific value—instead generate or inject the correct origin per
environment during global test setup (e.g., read from an ENV/CONFIG and populate
the "origin" field when creating the storage state) and remove the hardcoded
"origin" string from auth.json so tests can run against qa/prod without state
reuse issues.
In `@playwright/Actions/api/token.manager.ts`:
- Around line 20-43: The temporary APIRequestContext created by
request.newContext(...) inside the token manager method must always be disposed
even on error: wrap the usage of context (the call to
context.post(API_ROUTES.login.route, ...), response handling and await
response.json()) in a try/finally where context.dispose() is called from the
finally block so the context is released on success or on exceptions; keep the
existing checks that throw when !response.ok() and still set this.tokens from
the parsed body before returning.
In `@playwright/global.setup.ts`:
- Around line 124-125: The code reuses a single startTime for multiple
sequential polling phases (submission polling, static scan, PDF, privacy, SBOM),
which causes later phases to inherit whatever time budget prior phases consumed;
fix by giving each polling phase its own timeout budget—either reinitialize
startTime (or a per-phase startTimestamp) immediately before each polling loop
or compute and pass an explicit perPhaseTimeout constant to each loop; update
the occurrences around the startTime variable and the loops that reference it
(the initialization where startTime and fileId are set plus the other instances
mentioned) so each polling phase (submission, static scan, PDF generation,
privacy check, SBOM) uses its own start timestamp or timeout variable.
- Around line 8-14: The code currently hardcodes dotenv.config to load
'../.env.qa' while STATE_FILE uses process.env.ENVIRONMENT, causing mismatched
environments; change to compute a single env variable (e.g., const env =
process.env.ENVIRONMENT || 'qa') and use it for both dotenv.config and
STATE_FILE (call dotenv.config({ path: path.resolve(__dirname, `../.env.${env}`)
}) and build STATE_FILE with `${env}-state.json`), ensuring STATE_DIR,
STATE_FILE and dotenv.config all reference the same env value.
- Line 336: The code sets profileId to projectId instead of using the project's
actual profile identifier; update the object where profileId is assigned
(currently "profileId: projectId") to use mfvaProject.activeProfileId so
state.profileId references the correct profile for downstream calls like
dsManualDevicePreference and apiScanOptions; locate the creation of the state
object that includes profileId and replace the aliasing to projectId with
mfvaProject.activeProfileId.
In `@playwright/specs/api/auth.api.spec.ts`:
- Around line 204-223: The test 'GET /api/users/:id — invalid user id returns
200' incorrectly treats a response that returns the caller's own record as a
valid negative case; update the test (around the wrapper.get call and the
ResponseValidator.validate/assertion block) to reflect the intended contract:
either assert the API returns a not-found/error status for an invalid id (e.g.,
expect 404 via ResponseValidator.validate) or, if the real contract is "always
return current user", rename the test and assert returned data.id === userId;
reference wrapper.get({ endpoint: '/api/users/999999' }),
ResponseValidator.validate(...), and the userId variable when making the change.
- Around line 13-18: The suite-level RequestWrapper called "wrapper" is being
initialized with authentication in test.beforeAll via wrapper.init() /
TokenManager.getTokens(), causing the /api/login tests to run with auth headers;
change those login tests to use a fresh unauthenticated client instead of the
suite wrapper (either instantiate new RequestWrapper without calling init(), or
use/extend the existing pattern of creating testWrapper) so login requests do
not include setToken() headers; update the login test cases (the blocks that
currently call wrapper for POST /api/login) to use the new unauthenticated
instance and ensure any helper methods rely on the unauthenticated client.
In `@playwright/specs/api/dynamic-scan.api.spec.ts`:
- Around line 267-273: The device selection in dynamic-scan.api.spec.ts
currently picks devices by checking state === 'available' but must match the
predicate used in playwright/utils/dynamic-scan.utils.ts (which requires
is_reserved === false as well); update the predicate in the device lookup(s)
(e.g., the block that sets deviceIdentifier and the other similar selection
later in the file) to require both d.state === 'available' && d.is_reserved ===
false so tests don't pick reserved devices.
- Around line 390-409: The PUT that resets ds_manual_device_selection (currently
in the standalone test using wrapper.put with
resolveRoute(API_ROUTES.dsManualDevicePreference.route, state.profileId)) must
be moved out of a serial test and into guaranteed cleanup (either an afterAll
for the describe block that contains the DAST flow or a try/finally surrounding
the entire flow); locate the test named "PUT preference — reset to any available
device" and relocate its wrapper.put call so the reset always runs (use the same
endpoint and body) in afterAll or the finally block to ensure the profile is
unpinned even if earlier tests fail.
In `@playwright/specs/api/projects.api.spec.ts`:
- Around line 55-57: The allure.step calls call
ResponseValidator.validatePaginated(...) without awaiting it, so the step may
finish before the validator completes; update both occurrences (the step at
ResponseValidator.validatePaginated near the first instance and the one around
the second instance) to await ResponseValidator.validatePaginated(...) so the
step waits for the async validation to finish and propagate failures correctly.
In `@playwright/specs/api/sbom.api.spec.ts`:
- Around line 164-182: The polling loop in the allure.step reads body.pdf_status
without verifying the HTTP status from wrapper.get
(resolveRoute/API_ROUTES.sbReportById.route with state.sbReportId); change the
loop to first check response.status (or response.ok) and immediately throw a
descriptive error (including status and any error text) for non-200 responses so
the test fails fast instead of waiting the full TIMEOUT; keep the rest of the
pdf_status polling logic intact once a successful response is confirmed.
In `@playwright/specs/api/upload.api.spec.ts`:
- Around line 219-221: The test logs the entire response body in the allure.step
(calls to response.text() and console.log('invalid org response:', text)), which
can leak presigned S3 URLs and signed fields; change the step to avoid dumping
the raw body — either remove the console.log entirely or replace it with a safe
summary (e.g., response.status(), response.headers(), or the body length/JSON
keys) and redact any sensitive fields if you must inspect JSON. Update the
allure.step that currently calls response.text() to not expose the full text and
reference the same step name and response variable so the change is localized.
- Around line 197-228: The test currently asserts the buggy 200 success code,
locking the incorrect contract; update the test case (test named 'GET upload URL
with invalid org ID — returns 404 //orgId not valid uses token') to expect a 404
by changing the ResponseValidator.validate call to assert status: 404, and
update any related allure.step descriptions or messages (the step 'Validate
status 200 (BUG — should be 404)' and the step name that calls
wrapper.get/resolveRoute with the invalid org ID) so they reflect the correct
expected behavior; keep the GET request via wrapper.get and
resolveRoute(API_ROUTES.uploadApp.route, 7787878787) unchanged.
- Around line 68-86: The fetch to S3 (the call that produces s3Response using
s3Url and fileBuffer in the test step) needs an explicit timeout: create an
AbortController, start a timer (e.g., 30s) that calls controller.abort(), pass
controller.signal to fetch, and clear the timer after fetch resolves; also
handle abort errors so the test fails fast with a clear message instead of
hanging. Update the PUT block around the fetch(s3Url, { method: 'PUT', body:
fileBuffer, ... }) to include the signal and the abort/timer lifecycle so
s3Response is always resolved or rejected within the timeout.
In `@playwright/specs/auth.spec.ts`:
- Around line 126-171: The test registers mocks via the NetworkActions instance
(networkActions.mockNetworkReq) but only calls networkActions.clearAll() and
context.close() on the success path; wrap the interaction and assertions in a
try/finally where in the finally block you always call networkActions.clearAll()
and await context.close() (referencing the networkActions variable,
NetworkActions.clearAll method, and the context.close() call) so mocked routes
are cleared and the browser context is closed even when assertions fail.
In `@playwright/support/constants.ts`:
- Around line 31-40: SCAN_RUNNING_STATUSES currently omits the autopilot-phase
statuses so quick scans can pass READY_FOR_INTERACTION and be treated as not
running; update the SCAN_RUNNING_STATUSES array to include the
DYNAMIC_SCAN_STATUS enum values that correspond to the autopilot phases (the
status codes 12–15) so the running-state check in dynamic-scan.utils.ts
recognizes those phases as active; ensure you add the specific
DYNAMIC_SCAN_STATUS members for all autopilot states to the array (alongside
existing members) so fast scans are not timed out as “not running.”
In `@playwright/support/test-state.ts`:
- Around line 4-5: The env resolution in test-state.ts (variables env and
statePath) must be aligned with playwright.config.ts and global.setup.ts; change
env to read from the same variable chain used in config (e.g.,
process.env.TEST_ENV) with a fallback to process.env.ENVIRONMENT and finally
'qa' so statePath uses the identical environment value; apply the same
resolution logic in global.setup.ts (and confirm playwright.config.ts still
reads process.env.TEST_ENV) so all three files (env, statePath, and
global.setup) are consistent.
In `@playwright/utils/dynamic-scan.utils.ts`:
- Around line 76-80: The current code in dynamic-scan.utils.ts treats any
response.status() === 404 as “no active scan”, but because request.wrapper
returns raw responses for all endpoints, a 404 from a wrong fileId or route
regression will be silently accepted; update the logic in the function that
checks response.status() (the block using response.status() === 404) to only
treat 404 as “no active scan” when this response is confirmed to come from the
exact dynamic-scan endpoint (e.g., verify response.url() contains the expected
path) or when the payload/body explicitly signals “no active scan” (parse
response.json() and check the discriminating field), otherwise throw/return an
error (fail fast) so callers know the 404 was unexpected. Ensure you reference
the same response variable and keep behavior unchanged for genuine endpoint
404s.
---
Outside diff comments:
In `@package.json`:
- Around line 1-256: The package-lock.json is out of sync causing CI failure on
npm ci; run npm install locally to regenerate package-lock.json (ensuring
Node/npm versions match engines/volta constraints), verify no unexpected
changes, then add and commit the updated package-lock.json so the pipeline can
pass; reference package.json (engines/volta) to ensure compatible Node/npm when
reproducing.
---
Minor comments:
In `@playwright/global.setup.ts`:
- Around line 186-191: The lookup using analysisBody.results.find(...) can
return undefined, so guard the result (riskyAnalysis) before dereferencing; if
it's undefined throw a descriptive Error (e.g., "No risky analysis found in
analysisBody.results") during setup instead of letting a TypeError surface.
Update the block around the find call to check riskyAnalysis and then set
analysisId and vulnerabilityId only after the check, referencing the
riskyAnalysis variable and the analysisId and vulnerabilityId assignments in
your change.
In `@playwright/specs/api/auth.api.spec.ts`:
- Around line 118-149: This test instantiates a new RequestWrapper (testWrapper)
and never releases its request context; either remove the new RequestWrapper and
reuse the suite-level wrapper (e.g., use the existing
requestWrapper/requestSuiteWrapper instead of new RequestWrapper()) or wrap the
block that uses testWrapper in try/finally and call the wrapper's teardown
method in finally (e.g., await testWrapper.dispose()/close()/teardown()) to
ensure the request context is always released even on assertion failure; update
references to RequestWrapper.init and RequestWrapper.get accordingly when
reusing the suite wrapper.
In `@playwright/specs/api/report.api.spec.ts`:
- Around line 160-163: The test skip message contains a typo: replace the string
'PRIVACY SHEILD NOT ENABLED ON THIS ENVIRONMENT' used in the test.skip call with
the corrected 'PRIVACY SHIELD NOT ENABLED ON THIS ENVIRONMENT' (locate the
test.skip invocation in report.api.spec.ts and update the message literal).
- Around line 18-20: The afterAll cleanup in this spec only calls
wrapper.dispose() and omits clearing test tokens; update the test.afterAll block
to also call TokenManager.clearTokens() after (or before) awaiting
wrapper.dispose() so tokens are removed between runs—modify the test.afterAll in
report.api.spec.ts to invoke TokenManager.clearTokens() alongside the existing
wrapper.dispose() call.
In `@playwright/support/api.routes.ts`:
- Around line 145-148: Rename the mistyped route key sbReortCyclonedx to
sbReportCyclonedx so it matches the alias sbReportCyclonedx and avoids
confusion; update the object entry that contains route:
'/api/v2/sb_reports/*/cyclonedx_json_file/download_url' and alias:
'sbReportCyclonedx' to use the corrected key name sbReportCyclonedx.
In `@playwright/support/translations.ts`:
- Around line 10-21: getMessageFromTranslations currently returns only the last
unmatched segment (variable `c`) when a nested translation is missing, losing
context; change it to return the original full key string instead. Locate the
function getMessageFromTranslations and EN_TRANSLATIONS, keep the reduce
traversal but ensure the final result is validated: if the reduce doesn't
resolve to a string (or the path is missing), return the original `key` (the
full dotted path) as the fallback rather than `c`; alternatively propagate a
sentinel object through reduce and then map that to `key` before returning.
In `@playwright/support/utils.ts`:
- Around line 86-90: The function getVulnerabilityTypeText currently declares a
string return but may return undefined when vulnType is not one of the keys in
typeTextMap; update the implementation to either (A) widen the signature to
return string | undefined and keep the current lookup, or (B) validate vulnType
against the keys in typeTextMap (e.g., via Object.prototype.hasOwnProperty or a
map lookup) and return a fallback like 'unknown' or throw a controlled error —
apply the change to the getVulnerabilityTypeText function and adjust any
callers/tests that rely on a guaranteed string accordingly.
In `@playwright/utils/schema.validator.ts`:
- Around line 46-51: The validator currently uses expect(typeof value ...) to
check types which treats null as 'object'; update the check in the branch that
uses value, field and rules.type so that it first explicitly fails when value
=== null for expected object types (i.e., if rules.type === 'object' and value
=== null, call expect to fail with the same schema violation message), otherwise
continue with the typeof check; modify the block around the expect(...) using
value/field/rules.type to include this null guard so null does not pass object
validation.
---
Nitpick comments:
In @.gitignore:
- Around line 52-61: The .gitignore contains a duplicate node_modules entry:
remove the redundant "node_modules/" line (the unanchored pattern) since
"/node_modules/" at the repo root already covers it; if you intentionally need
to ignore nested node_modules directories, replace or clarify by keeping the
unanchored "node_modules/" and removing the anchored "/node_modules/"
instead—update the .gitignore accordingly around the existing node_modules
entries.
In `@playwright.config.ts`:
- Around line 1-46: Remove the large commented-out Playwright config block (the
46-line section containing imports for defineConfig, dotenv, path and the
commented env/config export) to eliminate dead code; specifically delete the
commented lines that reference defineConfig, dotenv.config, the env variable,
the export default defineConfig({...}) block and its nested reporter/use
settings so the file contains only active configuration or concise alternative
config.
- Line 60: The ternary expression for the Playwright config retries is
redundant: replace the `retries: process.env.CI ? 1 : 1` entry in the Playwright
configuration with a single value (e.g., `retries: 1`) or implement distinct
values for CI vs local runs if intended; update the retries property in the
Playwright config object (the line containing `retries: process.env.CI ? 1 : 1`)
accordingly so the branch is no longer redundant.
In `@playwright/Actions/api/request.wrapper.ts`:
- Around line 5-9: The RequestOptions interface declares requiresAuth but it's
never used; remove the requiresAuth property from RequestOptions declared in
request.wrapper.ts and update any call sites that pass requiresAuth to stop
supplying it (or remove the property from object literals) so the type and
usages stay consistent; if you prefer to keep the option instead, implement
conditional auth handling inside the module's request-sending function (e.g.,
add an Authorization header when RequestOptions.requiresAuth is true) and ensure
RequestOptions is read where requests are constructed.
In `@playwright/Actions/auth/loginActions.ts`:
- Around line 14-16: Replace the hardcoded "Next" label with the localized
translation: change the getByRole call that assigns nextButton
(page.getByRole('button', { name: 'Next' })) to use pwTranslate('next') so the
button lookup uses the translation key; keep the rest of the flow
(expect(nextButton).toBeEnabled() and nextButton.click()) unchanged and ensure
pwTranslate is imported/available in loginActions.ts.
In `@playwright/specs/api/report.api.spec.ts`:
- Around line 153-158: The nested describe is defensively re-initializing the
RequestWrapper even though the outer test.beforeAll already sets up wrapper;
remove the redundant initialization block inside the nested describe (the
beforeAll that constructs and calls wrapper.init()) so the tests reuse the outer
wrapper, and if you decide to keep a nested setup, add a matching afterAll that
calls wrapper.dispose() (or the appropriate teardown method on RequestWrapper)
to avoid leaking resources; reference the symbols wrapper, RequestWrapper,
beforeAll, and afterAll when making the change.
In `@playwright/specs/api/scan.api.spec.ts`:
- Around line 81-85: Remove the unnecessary dynamic import of expect inside the
allure.step: add expect to the existing static import from '@playwright/test' at
the top of the file (where test is imported), then delete the dynamic "const {
expect } = await import('@playwright/test');" line and use the top-level expect
in the allure.step assertions (the block referencing body.static_scan_progress
and body.is_static_done remains unchanged).
In `@playwright/support/api.routes.ts`:
- Around line 307-316: The resolveRoute helper silently strips unmatched '*'
wildcards which can mask caller bugs; update resolveRoute to validate that the
number of provided params matches the number of wildcards in the route (e.g.,
count '*' in the incoming route) before doing replacements, and if they don't
match either throw a descriptive Error or at minimum log a warning via your
logger; keep the existing replacement logic (replacing '*' with String(param) in
the loop over params) but remove the final unconditional replace(/\*/g,'') and
instead fail fast when leftover wildcards would remain so callers get an
immediate, debuggable failure.
In `@playwright/support/test-state.ts`:
- Around line 27-28: Wrap the module-level file read/parse that initializes the
`state: TestState` from `statePath` (the `fs.readFileSync` + `JSON.parse`) in a
try-catch; on error (especially ENOENT) throw a new Error with a clear message
that the state file at `statePath` is missing or failed to parse and advise
running global setup, and include the original error details for debugging;
ensure `state` is still exported as the default after successful parse.
In `@playwright/utils/response.validator.ts`:
- Around line 30-31: The call to response.json() in response.validator.ts can
throw for non-JSON or empty responses; wrap the await response.json() in a
try-catch inside the validator (around the expression using response.json()) and
handle failures by either attempting response.text() as a fallback or setting
body to null/undefined, then include a clear error message that includes
response.status and response.url (or the response object) when rethrowing or
asserting so test failures show that the body was non-JSON rather than crashing
with an unhelpful parse error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d36aa3c2-8532-4851-bd00-8205185d74cc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.gitignoreauth.jsoncypress/support/Actions/auth/LoginActions.tsmirage/factories/sbom-component.tspackage.jsonplaywright.config.tsplaywright/Actions/api/api-client.tsplaywright/Actions/api/request.wrapper.tsplaywright/Actions/api/token.manager.tsplaywright/Actions/auth/loginActions.tsplaywright/Actions/network.actions.tsplaywright/fixtures/DVIA.ipaplaywright/fixtures/MFVA.aabplaywright/fixtures/MFVA.apkplaywright/global.setup.tsplaywright/specs/api/auth.api.spec.tsplaywright/specs/api/dynamic-scan.api.spec.tsplaywright/specs/api/edit-analysis.api.spec.tsplaywright/specs/api/projects.api.spec.tsplaywright/specs/api/report.api.spec.tsplaywright/specs/api/sbom.api.spec.tsplaywright/specs/api/scan.api.spec.tsplaywright/specs/api/tags.api.spec.tsplaywright/specs/api/upload.api.spec.tsplaywright/specs/api/vulnerability.api.spec.tsplaywright/specs/auth.spec.tsplaywright/support/api.routes.tsplaywright/support/application.routes.tsplaywright/support/constants.tsplaywright/support/test-state.tsplaywright/support/translations.tsplaywright/support/utils.tsplaywright/ui.setup.spec.tsplaywright/utils/dynamic-scan.utils.tsplaywright/utils/response.validator.tsplaywright/utils/schema.validator.ts
💤 Files with no reviewable changes (1)
- cypress/support/Actions/auth/LoginActions.ts
| "cookies": [], | ||
| "origins": [ | ||
| { | ||
| "origin": "https://sherlockprivacy.qa.do.appknox.io", |
There was a problem hiding this comment.
Avoid hardcoded origin in persisted auth state.
Line 5 hardcodes a QA host, which undermines the multi-environment (qa/prod) goal and makes state reuse fragile across environments.
🔧 Suggested direction
- "origin": "https://sherlockprivacy.qa.do.appknox.io",
+ "origin": "${BASE_URL_FROM_ENV}",Generate storage state per environment during global setup instead of committing environment-specific state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@auth.json` at line 5, The persisted auth state currently hardcodes the
"origin" value to a QA host; update the setup so the "origin" field is not
committed with an environment-specific value—instead generate or inject the
correct origin per environment during global test setup (e.g., read from an
ENV/CONFIG and populate the "origin" field when creating the storage state) and
remove the hardcoded "origin" string from auth.json so tests can run against
qa/prod without state reuse issues.
Comment on lines
+8
to
+9
| "name": "ember_simple_auth-session", | ||
| "value": "{\"authenticated\":{\"authenticator\":\"authenticator:irene\",\"token\":\"653b5bbdfffd105d36fce72207778e3b060f379bc87316ca18334e62988fa11f\",\"user_id\":1,\"b64token\":\"MTo2NTNiNWJiZGZmZmQxMDVkMzZmY2U3MjIwNzc3OGUzYjA2MGYzNzliYzg3MzE2Y2ExODMzNGU2Mjk4OGZhMTFm\"}}" |
There was a problem hiding this comment.
Remove committed session secrets from repository history.
Line 9 includes live auth material (token, b64token, user_id) inside Playwright storage state. This is a secret/identity leak and should not be committed.
🔧 Suggested fix
{
"cookies": [],
- "origins": [
- {
- "origin": "https://sherlockprivacy.qa.do.appknox.io",
- "localStorage": [
- {
- "name": "ember_simple_auth-session",
- "value": "{\"authenticated\":{\"authenticator\":\"authenticator:irene\",\"token\":\"...\",\"user_id\":1,\"b64token\":\"...\"}}"
- }
- ]
- }
- ]
+ "origins": []
}Also ensure auth state files are generated at runtime in global setup and ignored by git (e.g., add auth-state paths to .gitignore), then rotate/revoke the exposed token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@auth.json` around lines 8 - 9, The committed Playwright storage state
contains live secrets under the "ember_simple_auth-session" JSON (keys "token",
"b64token", "user_id"); remove these values from the committed auth JSON,
rotate/revoke the exposed token immediately, and purge the secret from repo
history (use git filter-repo or BFG). Add the auth-state filename(s) to
.gitignore and modify your test bootstrap/globalSetup routine (e.g., the
globalSetup function that creates storage state) to generate the Playwright
storage state at runtime instead of committing it. Ensure the sanitized
committed file contains no credentials and that runtime-generated state is used
in CI/local runs.
Comment on lines
+20
to
+43
| const context = await request.newContext({ | ||
| baseURL: process.env.BASE_URL!, | ||
| }); | ||
|
|
||
| const response = await context.post(API_ROUTES.login.route, { | ||
| data: { | ||
| username: process.env.TEST_USERNAME!, | ||
| password: process.env.TEST_PASSWORD!, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok()) { | ||
| throw new Error(`Login failed with status ${response.status()}`); | ||
| } | ||
|
|
||
| const body = await response.json(); | ||
| this.tokens = { | ||
| token: body.token, | ||
| user_id: body.user_id, | ||
| }; | ||
|
|
||
| await context.dispose(); | ||
|
|
||
| return this.tokens; |
There was a problem hiding this comment.
Dispose the temporary login context in a finally.
A failed login path exits before context.dispose(), so network/JSON errors leak an APIRequestContext from the shared auth bootstrap.
♻️ Proposed fix
const context = await request.newContext({
baseURL: process.env.BASE_URL!,
});
-
- const response = await context.post(API_ROUTES.login.route, {
- data: {
- username: process.env.TEST_USERNAME!,
- password: process.env.TEST_PASSWORD!,
- },
- });
-
- if (!response.ok()) {
- throw new Error(`Login failed with status ${response.status()}`);
- }
-
- const body = await response.json();
- this.tokens = {
- token: body.token,
- user_id: body.user_id,
- };
-
- await context.dispose();
-
- return this.tokens;
+ try {
+ const response = await context.post(API_ROUTES.login.route, {
+ data: {
+ username: process.env.TEST_USERNAME!,
+ password: process.env.TEST_PASSWORD!,
+ },
+ });
+
+ if (!response.ok()) {
+ throw new Error(`Login failed with status ${response.status()}`);
+ }
+
+ const body = await response.json();
+ this.tokens = {
+ token: body.token,
+ user_id: body.user_id,
+ };
+
+ return this.tokens;
+ } finally {
+ await context.dispose();
+ }📝 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
| const context = await request.newContext({ | |
| baseURL: process.env.BASE_URL!, | |
| }); | |
| const response = await context.post(API_ROUTES.login.route, { | |
| data: { | |
| username: process.env.TEST_USERNAME!, | |
| password: process.env.TEST_PASSWORD!, | |
| }, | |
| }); | |
| if (!response.ok()) { | |
| throw new Error(`Login failed with status ${response.status()}`); | |
| } | |
| const body = await response.json(); | |
| this.tokens = { | |
| token: body.token, | |
| user_id: body.user_id, | |
| }; | |
| await context.dispose(); | |
| return this.tokens; | |
| const context = await request.newContext({ | |
| baseURL: process.env.BASE_URL!, | |
| }); | |
| try { | |
| const response = await context.post(API_ROUTES.login.route, { | |
| data: { | |
| username: process.env.TEST_USERNAME!, | |
| password: process.env.TEST_PASSWORD!, | |
| }, | |
| }); | |
| if (!response.ok()) { | |
| throw new Error(`Login failed with status ${response.status()}`); | |
| } | |
| const body = await response.json(); | |
| this.tokens = { | |
| token: body.token, | |
| user_id: body.user_id, | |
| }; | |
| return this.tokens; | |
| } finally { | |
| await context.dispose(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/Actions/api/token.manager.ts` around lines 20 - 43, The temporary
APIRequestContext created by request.newContext(...) inside the token manager
method must always be disposed even on error: wrap the usage of context (the
call to context.post(API_ROUTES.login.route, ...), response handling and await
response.json()) in a try/finally where context.dispose() is called from the
finally block so the context is released on success or on exceptions; keep the
existing checks that throw when !response.ok() and still set this.tokens from
the parsed body before returning.
Comment on lines
+8
to
+14
| dotenv.config({ path: path.resolve(__dirname, '../.env.qa') }); | ||
|
|
||
| const STATE_DIR = path.resolve(__dirname, '../.state'); | ||
| const STATE_FILE = path.join( | ||
| STATE_DIR, | ||
| `${process.env.ENVIRONMENT || 'qa'}-state.json` | ||
| ); |
There was a problem hiding this comment.
Resolve the dotenv file from the active environment.
The module always loads .env.qa, but the state filename is derived from ENVIRONMENT. A non-QA run can therefore emit the right *-state.json name while still using QA config. Use one env selector for both.
🛠️ Possible fix
-dotenv.config({ path: path.resolve(__dirname, '../.env.qa') });
+const env = process.env.ENVIRONMENT ?? process.env.TEST_ENV ?? 'qa';
+dotenv.config({ path: path.resolve(__dirname, `../.env.${env}`) });
const STATE_DIR = path.resolve(__dirname, '../.state');
const STATE_FILE = path.join(
STATE_DIR,
- `${process.env.ENVIRONMENT || 'qa'}-state.json`
+ `${env}-state.json`
);📝 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
| dotenv.config({ path: path.resolve(__dirname, '../.env.qa') }); | |
| const STATE_DIR = path.resolve(__dirname, '../.state'); | |
| const STATE_FILE = path.join( | |
| STATE_DIR, | |
| `${process.env.ENVIRONMENT || 'qa'}-state.json` | |
| ); | |
| const env = process.env.ENVIRONMENT ?? process.env.TEST_ENV ?? 'qa'; | |
| dotenv.config({ path: path.resolve(__dirname, `../.env.${env}`) }); | |
| const STATE_DIR = path.resolve(__dirname, '../.state'); | |
| const STATE_FILE = path.join( | |
| STATE_DIR, | |
| `${env}-state.json` | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/global.setup.ts` around lines 8 - 14, The code currently hardcodes
dotenv.config to load '../.env.qa' while STATE_FILE uses
process.env.ENVIRONMENT, causing mismatched environments; change to compute a
single env variable (e.g., const env = process.env.ENVIRONMENT || 'qa') and use
it for both dotenv.config and STATE_FILE (call dotenv.config({ path:
path.resolve(__dirname, `../.env.${env}`) }) and build STATE_FILE with
`${env}-state.json`), ensuring STATE_DIR, STATE_FILE and dotenv.config all
reference the same env value.
Comment on lines
+124
to
+125
| const startTime = Date.now(); | ||
| let fileId: number = 0; |
There was a problem hiding this comment.
Give each polling phase its own timeout budget.
startTime is initialized once before submission polling and then reused for static scan, PDF, privacy, and SBOM waits. If the early phases consume most of the 10 minutes, the later loops time out almost immediately even though their own work has just started.
Also applies to: 166-167, 225-226, 257-258, 299-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/global.setup.ts` around lines 124 - 125, The code reuses a single
startTime for multiple sequential polling phases (submission polling, static
scan, PDF, privacy, SBOM), which causes later phases to inherit whatever time
budget prior phases consumed; fix by giving each polling phase its own timeout
budget—either reinitialize startTime (or a per-phase startTimestamp) immediately
before each polling loop or compute and pass an explicit perPhaseTimeout
constant to each loop; update the occurrences around the startTime variable and
the loops that reference it (the initialization where startTime and fileId are
set plus the other instances mentioned) so each polling phase (submission,
static scan, PDF generation, privacy check, SBOM) uses its own start timestamp
or timeout variable.
Comment on lines
+219
to
+221
| await allure.step('Log raw response body', async () => { | ||
| const text = await response.text(); | ||
| console.log('invalid org response:', text); |
There was a problem hiding this comment.
Avoid logging the raw upload-init body.
On this bug path the response can include a presigned S3 URL plus signed upload fields. Dumping the full body to CI/Allure leaks short-lived credentials into logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/specs/api/upload.api.spec.ts` around lines 219 - 221, The test
logs the entire response body in the allure.step (calls to response.text() and
console.log('invalid org response:', text)), which can leak presigned S3 URLs
and signed fields; change the step to avoid dumping the raw body — either remove
the console.log entirely or replace it with a safe summary (e.g.,
response.status(), response.headers(), or the body length/JSON keys) and redact
any sensitive fields if you must inspect JSON. Update the allure.step that
currently calls response.text() to not expose the full text and reference the
same step name and response variable so the change is localized.
Comment on lines
+126
to
+171
| const context = await browser.newContext({ | ||
| storageState: { cookies: [], origins: [] }, | ||
| }); | ||
|
|
||
| const page = await context.newPage(); | ||
| const networkActions = new NetworkActions(page); | ||
|
|
||
| await allure.step('Mock login API to return 401', async () => { | ||
| await networkActions.mockNetworkReq({ | ||
| method: 'POST', | ||
| route: API_ROUTES.login.route, | ||
| status: 401, | ||
| dataOverride: { | ||
| message: 'The credentials you entered are incorrect', | ||
| attempt_left: '4', | ||
| failure_limit: '5', | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await allure.step('Navigate to login page', async () => { | ||
| await page.goto(APPLICATION_ROUTES.login); | ||
| }); | ||
|
|
||
| await allure.step('Fill username and click Next', async () => { | ||
| await page | ||
| .getByPlaceholder(pwTranslate('usernameEmailIdTextPlaceholder')) | ||
| .fill(process.env.TEST_USERNAME!); | ||
| await page.getByRole('button', { name: 'Next' }).click(); | ||
| }); | ||
|
|
||
| await allure.step('Fill wrong password and click Login', async () => { | ||
| await page | ||
| .getByPlaceholder(pwTranslate('passwordPlaceholder')) | ||
| .fill('wrong_password'); | ||
| await page.locator('button[aria-label="login-submit-button"]').click(); | ||
| }); | ||
|
|
||
| await allure.step('Verify error message is visible', async () => { | ||
| await expect( | ||
| page.getByText(pwTranslate('credentialsIncorrect')) | ||
| ).toBeVisible(); | ||
| }); | ||
|
|
||
| await networkActions.clearAll(); | ||
| await context.close(); |
There was a problem hiding this comment.
Always clear mocked routes in a finally block.
NetworkActions.clearAll() is the only cleanup path for route mocks (playwright/Actions/network.actions.ts:82-88), but here it runs only if the assertions all pass. A failure before Lines 170-171 can leave the mocked login route registered for later tests in the same worker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/specs/auth.spec.ts` around lines 126 - 171, The test registers
mocks via the NetworkActions instance (networkActions.mockNetworkReq) but only
calls networkActions.clearAll() and context.close() on the success path; wrap
the interaction and assertions in a try/finally where in the finally block you
always call networkActions.clearAll() and await context.close() (referencing the
networkActions variable, NetworkActions.clearAll method, and the context.close()
call) so mocked routes are cleared and the browser context is closed even when
assertions fail.
Comment on lines
+31
to
+40
| export const SCAN_RUNNING_STATUSES: number[] = [ | ||
| DYNAMIC_SCAN_STATUS.DEVICE_ALLOCATED, | ||
| DYNAMIC_SCAN_STATUS.CONNECTING_TO_DEVICE, | ||
| DYNAMIC_SCAN_STATUS.PREPARING_DEVICE, | ||
| DYNAMIC_SCAN_STATUS.INSTALLING, | ||
| DYNAMIC_SCAN_STATUS.CONFIGURING_API_CAPTURE, | ||
| DYNAMIC_SCAN_STATUS.HOOKING, | ||
| DYNAMIC_SCAN_STATUS.LAUNCHING, | ||
| DYNAMIC_SCAN_STATUS.READY_FOR_INTERACTION, | ||
| ]; |
There was a problem hiding this comment.
Include the autopilot phases in the running-status set.
playwright/utils/dynamic-scan.utils.ts:51-58 uses SCAN_RUNNING_STATUSES as the definition of an active scan. Omitting statuses 12-15 means a fast scan can advance past READY_FOR_INTERACTION before the next poll and still time out as “not running”.
🛠️ Possible fix
export const SCAN_RUNNING_STATUSES: number[] = [
DYNAMIC_SCAN_STATUS.DEVICE_ALLOCATED,
DYNAMIC_SCAN_STATUS.CONNECTING_TO_DEVICE,
DYNAMIC_SCAN_STATUS.PREPARING_DEVICE,
DYNAMIC_SCAN_STATUS.INSTALLING,
DYNAMIC_SCAN_STATUS.CONFIGURING_API_CAPTURE,
DYNAMIC_SCAN_STATUS.HOOKING,
DYNAMIC_SCAN_STATUS.LAUNCHING,
DYNAMIC_SCAN_STATUS.READY_FOR_INTERACTION,
+ DYNAMIC_SCAN_STATUS.DOWNLOADING_AUTOPILOT_SCRIPT,
+ DYNAMIC_SCAN_STATUS.CONFIGURING_AUTOPILOT,
+ DYNAMIC_SCAN_STATUS.AUTOPILOT_RUNNING,
+ DYNAMIC_SCAN_STATUS.AUTOPILOT_COMPLETED,
];📝 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
| export const SCAN_RUNNING_STATUSES: number[] = [ | |
| DYNAMIC_SCAN_STATUS.DEVICE_ALLOCATED, | |
| DYNAMIC_SCAN_STATUS.CONNECTING_TO_DEVICE, | |
| DYNAMIC_SCAN_STATUS.PREPARING_DEVICE, | |
| DYNAMIC_SCAN_STATUS.INSTALLING, | |
| DYNAMIC_SCAN_STATUS.CONFIGURING_API_CAPTURE, | |
| DYNAMIC_SCAN_STATUS.HOOKING, | |
| DYNAMIC_SCAN_STATUS.LAUNCHING, | |
| DYNAMIC_SCAN_STATUS.READY_FOR_INTERACTION, | |
| ]; | |
| export const SCAN_RUNNING_STATUSES: number[] = [ | |
| DYNAMIC_SCAN_STATUS.DEVICE_ALLOCATED, | |
| DYNAMIC_SCAN_STATUS.CONNECTING_TO_DEVICE, | |
| DYNAMIC_SCAN_STATUS.PREPARING_DEVICE, | |
| DYNAMIC_SCAN_STATUS.INSTALLING, | |
| DYNAMIC_SCAN_STATUS.CONFIGURING_API_CAPTURE, | |
| DYNAMIC_SCAN_STATUS.HOOKING, | |
| DYNAMIC_SCAN_STATUS.LAUNCHING, | |
| DYNAMIC_SCAN_STATUS.READY_FOR_INTERACTION, | |
| DYNAMIC_SCAN_STATUS.DOWNLOADING_AUTOPILOT_SCRIPT, | |
| DYNAMIC_SCAN_STATUS.CONFIGURING_AUTOPILOT, | |
| DYNAMIC_SCAN_STATUS.AUTOPILOT_RUNNING, | |
| DYNAMIC_SCAN_STATUS.AUTOPILOT_COMPLETED, | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/support/constants.ts` around lines 31 - 40, SCAN_RUNNING_STATUSES
currently omits the autopilot-phase statuses so quick scans can pass
READY_FOR_INTERACTION and be treated as not running; update the
SCAN_RUNNING_STATUSES array to include the DYNAMIC_SCAN_STATUS enum values that
correspond to the autopilot phases (the status codes 12–15) so the running-state
check in dynamic-scan.utils.ts recognizes those phases as active; ensure you add
the specific DYNAMIC_SCAN_STATUS members for all autopilot states to the array
(alongside existing members) so fast scans are not timed out as “not running.”
Comment on lines
+4
to
+5
| const env = process.env.ENVIRONMENT || 'qa'; | ||
| const statePath = path.resolve(__dirname, `../../.state/${env}-state.json`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which env var global.setup.ts uses for the state file path
rg -n 'ENVIRONMENT|TEST_ENV' playwright/global.setup.tsRepository: appknox/irene
Length of output: 109
🏁 Script executed:
rg -n 'ENVIRONMENT|TEST_ENV' playwright/playwright.config.tsRepository: appknox/irene
Length of output: 128
🏁 Script executed:
fd -name "playwright.config.*" -o -name "playwright.config.js"Repository: appknox/irene
Length of output: 287
🏁 Script executed:
fd "playwright.config"Repository: appknox/irene
Length of output: 77
🏁 Script executed:
rg -n 'ENVIRONMENT|TEST_ENV' playwright.config.tsRepository: appknox/irene
Length of output: 305
Environment variable inconsistency across config and state files.
playwright.config.ts uses process.env.TEST_ENV (line 51) while both global.setup.ts and test-state.ts use process.env.ENVIRONMENT to determine state file names. If TEST_ENV is set without ENVIRONMENT, the config may use a different env value than the state file path, causing the setup and test-state to become misaligned. Ensure both files use the same environment variable or that one consistently falls back to the other.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/support/test-state.ts` around lines 4 - 5, The env resolution in
test-state.ts (variables env and statePath) must be aligned with
playwright.config.ts and global.setup.ts; change env to read from the same
variable chain used in config (e.g., process.env.TEST_ENV) with a fallback to
process.env.ENVIRONMENT and finally 'qa' so statePath uses the identical
environment value; apply the same resolution logic in global.setup.ts (and
confirm playwright.config.ts still reads process.env.TEST_ENV) so all three
files (env, statePath, and global.setup) are consistent.
Comment on lines
+76
to
+80
| // 404 means no active scan → safe to start new one | ||
| if (response.status() === 404) { | ||
| console.log('[Utils] No active scan — safe to start new one'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don't treat every 404 as “scan stopped”.
playwright/Actions/api/request.wrapper.ts:31-39 returns raw responses for any status, so this branch also turns wrong-fileId and route-regression 404s into success. Only map 404 to “no active scan” if that behavior is guaranteed for this exact endpoint; otherwise fail fast with the unexpected status/payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright/utils/dynamic-scan.utils.ts` around lines 76 - 80, The current
code in dynamic-scan.utils.ts treats any response.status() === 404 as “no active
scan”, but because request.wrapper returns raw responses for all endpoints, a
404 from a wrong fileId or route regression will be silently accepted; update
the logic in the function that checks response.status() (the block using
response.status() === 404) to only treat 404 as “no active scan” when this
response is confirmed to come from the exact dynamic-scan endpoint (e.g., verify
response.url() contains the expected path) or when the payload/body explicitly
signals “no active scan” (parse response.json() and check the discriminating
field), otherwise throw/return an error (fail fast) so callers know the 404 was
unexpected. Ensure you reference the same response variable and keep behavior
unchanged for genuine endpoint 404s.
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
1 participant
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.
feat: playwright test framework - 90 tests passing