Add file upload e2e tests and e2e tests docs for AI agents#8404
Open
pa-lem wants to merge 3 commits intorelease-1.8from
Open
Add file upload e2e tests and e2e tests docs for AI agents#8404pa-lem wants to merge 3 commits intorelease-1.8from
pa-lem wants to merge 3 commits intorelease-1.8from
Conversation
github-actions
bot
added
the
group/frontend
Contributor
WalkthroughAdds a Playwright E2E testing guide at dev/guides/frontend/writing-e2e-tests.md and registers it in frontend/app/AGENTS.md. Introduces file upload test helpers at frontend/app/tests/e2e/objects/file-upload/file-upload-helpers.ts (upload utilities, test file generation, form filler) and a comprehensive test suite frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts covering branch-per-test-file setup, multiple authentication states (storageState presets), upload/edit/download workflows, validations, and role-based access. 🚥 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. 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 |
![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: 2
🧹 Nitpick comments (5)
frontend/app/tests/e2e/objects/file-upload/file-upload-helpers.ts (2)
36-44: SeveralTEST_FILE_TYPESentries are defined but never used.
PNG,JPEG, andCSVare declared but not referenced anywhere in the helpers or tests. If these are for future use, that's fine — just noting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tests/e2e/objects/file-upload/file-upload-helpers.ts` around lines 36 - 44, TEST_FILE_TYPES contains unused entries (PDF, PNG, JPEG, CSV); either remove these unused entries from the TEST_FILE_TYPES constant to keep the test helpers minimal, or if they are intentionally reserved for future tests, add a short comment above TEST_FILE_TYPES noting they are intentionally included for future use. Update the TEST_FILE_TYPES declaration (the constant named TEST_FILE_TYPES) to reflect the chosen approach so lint/tests no longer flag unused exports.
46-61: JSDoc says "random content" but the output is deterministic.The generated content is always the same repeated line for a given size. Consider updating the doc comment to say "Generate test file content of a given size" instead.
📝 Suggested doc fix
-/** - * Generate a test file with random content - */ +/** + * Generate deterministic test file content of a given size + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tests/e2e/objects/file-upload/file-upload-helpers.ts` around lines 46 - 61, Update the JSDoc for generateTestFileContent to accurately describe its behavior: replace "Generate a test file with random content" with something like "Generate test file content of an approximate size (deterministic repeated line)"; reference the function name generateTestFileContent and optionally mention sizes, targetSize, and line to clarify the docstring change so it no longer claims randomness.dev/guides/frontend/writing-e2e-tests.md (1)
286-292: Polling example lacks a note about timeout protection.The
whileloop has no explicit exit condition beyond the text disappearing. It works because Playwright's test timeout will eventually kill the test, but this isn't mentioned. A reader unfamiliar with the framework may not realize the safeguard, or may copy this pattern into a context without a timeout.Consider adding a brief note:
### Polling for Async Backend Processing +> The test's built-in timeout (60s default, 180s in CI) will terminate the loop if the condition never resolves. + ```typescript while (await page.getByText("No activity found").isVisible()) {Based on learnings: "prefer using the framework's built-in test timeout mechanism (60 seconds default, 180 seconds with CI=true) rather than implementing custom retry counters in polling loops."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev/guides/frontend/writing-e2e-tests.md` around lines 286 - 292, The polling example uses a bare while loop with page.getByText("No activity found").isVisible() and page.reload() but lacks any note about relying on Playwright's built-in test timeout for safety; update the documentation around this snippet (referencing page.getByText, isVisible, page.reload, and expect(...).toBeHidden()) to explicitly state that the loop relies on the framework test timeout (default 60s, 180s in CI) and recommend preferring that built-in timeout over custom retry counters, or alternatively show how to add an explicit timeout/abort mechanism if readers need one.frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts (2)
203-216: Overly broad selector and implicit data dependency make this test fragile.
page.getByRole("link").first()(Line 207) matches any link on the page — navigation, breadcrumbs, etc. — not necessarily a file object link. Additionally, this test silently passes if no link is visible, giving a false green.Consider scoping the selector to the list container and asserting that at least one item exists:
♻️ Suggested improvement
await test.step("navigate to an existing file object", async () => { await page.goto(`/objects/InfraCircuitContract?branch=${BRANCH_NAME}`); - const fileLink = page.getByRole("link").first(); - if (await fileLink.isVisible()) { - await fileLink.click(); - } + const listContainer = page.getByTestId("object-items"); + const fileLink = listContainer.getByRole("link").first(); + await expect(fileLink).toBeVisible(); + await fileLink.click(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts` around lines 203 - 216, The test "should not be able to edit existing file" uses an overly broad selector page.getByRole("link").first() and silently skips clicking if no link is visible; update the test to scope the file link lookup to the file list container (e.g., a locator like page.getByTestId("file-list") or a container-specific selector) and then assert that at least one file item exists before interacting (use an expectation like expect(locator.count() or locator.first()).toBeGreaterThan(0) / toBeVisible()), then click the scoped link; keep the final assertion on getByTestId("edit-button") toBeDisabled unchanged.
29-38: "Not logged in" test silently passes when create button is absent.If the button isn't rendered at all, the
ifbranch is skipped and the test asserts nothing. Consider asserting a concrete expectation — e.g., that the button is either not visible or disabled:♻️ Suggested improvement
test("should not be able to create file object", async ({ page }) => { await page.goto(`/objects/InfraCircuitContract?branch=${BRANCH_NAME}`); - // Create button should not be visible or enabled const createButton = page.getByTestId("create-object-button"); - if (await createButton.isVisible()) { - await expect(createButton).toBeDisabled(); - } + // Button should either be absent or disabled + await expect(createButton).not.toBeVisible(); });If the UI may render it as disabled instead of hidden, use a conditional with an explicit fallback assertion to ensure the test never silently passes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts` around lines 29 - 38, The test "should not be able to create file object" currently does nothing when createButton (getByTestId("create-object-button")) is absent; change it to assert a concrete condition by checking visibility and then asserting either not visible OR disabled: e.g., evaluate createButton.isVisible() and assert false if visible then assert createButton.isDisabled(), so the test fails if the button is present and enabled; update the assertions in that test body (referencing createButton and the test name) to guarantee a deterministic expectation instead of a silent pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts`:
- Around line 8-19: The outer test.describe "File Upload - InfraCircuitContract"
has dependent nested describes but no execution mode set; add a serial
configuration to that describe to enforce ordering (e.g., call
test.describe.configure({ mode: "serial" }) at the top of the describe block
that defines BRANCH_NAME/TEST_FILE_NAME/TEST_FILE_CONTENT) so the Admin-created
data (used by the Read-Only tests) is available when those tests run; update the
describe block containing BRANCH_NAME, TEST_FILE_NAME, TEST_FILE_CONTENT and the
beforeAll/afterAll hooks to include this configuration.
- Around line 249-261: The test currently skips assertions when the download
button is missing because the logic is wrapped in an if check; remove that
conditional and instead assert the button is present/visible (use the test
runner's expect on downloadButton or expect(await
downloadButton.isVisible()).toBeTruthy()) before attempting the download, then
proceed to call page.waitForEvent("download"), click the downloadButton, await
the download, and assert download.suggestedFilename() matches fileName; update
the block around downloadButton, page.waitForEvent, and
download.suggestedFilename to reflect this change.
---
Nitpick comments:
In `@dev/guides/frontend/writing-e2e-tests.md`:
- Around line 286-292: The polling example uses a bare while loop with
page.getByText("No activity found").isVisible() and page.reload() but lacks any
note about relying on Playwright's built-in test timeout for safety; update the
documentation around this snippet (referencing page.getByText, isVisible,
page.reload, and expect(...).toBeHidden()) to explicitly state that the loop
relies on the framework test timeout (default 60s, 180s in CI) and recommend
preferring that built-in timeout over custom retry counters, or alternatively
show how to add an explicit timeout/abort mechanism if readers need one.
In `@frontend/app/tests/e2e/objects/file-upload/file-upload-helpers.ts`:
- Around line 36-44: TEST_FILE_TYPES contains unused entries (PDF, PNG, JPEG,
CSV); either remove these unused entries from the TEST_FILE_TYPES constant to
keep the test helpers minimal, or if they are intentionally reserved for future
tests, add a short comment above TEST_FILE_TYPES noting they are intentionally
included for future use. Update the TEST_FILE_TYPES declaration (the constant
named TEST_FILE_TYPES) to reflect the chosen approach so lint/tests no longer
flag unused exports.
- Around line 46-61: Update the JSDoc for generateTestFileContent to accurately
describe its behavior: replace "Generate a test file with random content" with
something like "Generate test file content of an approximate size (deterministic
repeated line)"; reference the function name generateTestFileContent and
optionally mention sizes, targetSize, and line to clarify the docstring change
so it no longer claims randomness.
In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts`:
- Around line 203-216: The test "should not be able to edit existing file" uses
an overly broad selector page.getByRole("link").first() and silently skips
clicking if no link is visible; update the test to scope the file link lookup to
the file list container (e.g., a locator like page.getByTestId("file-list") or a
container-specific selector) and then assert that at least one file item exists
before interacting (use an expectation like expect(locator.count() or
locator.first()).toBeGreaterThan(0) / toBeVisible()), then click the scoped
link; keep the final assertion on getByTestId("edit-button") toBeDisabled
unchanged.
- Around line 29-38: The test "should not be able to create file object"
currently does nothing when createButton (getByTestId("create-object-button"))
is absent; change it to assert a concrete condition by checking visibility and
then asserting either not visible OR disabled: e.g., evaluate
createButton.isVisible() and assert false if visible then assert
createButton.isDisabled(), so the test fails if the button is present and
enabled; update the assertions in that test body (referencing createButton and
the test name) to guarantee a deterministic expectation instead of a silent
pass.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts (1)
21-27: 500-response guard produces a confusing failure message.When a 500 is caught, the assertion
expect(response.url()).toBe("This URL responded with a 500 status")will always fail — by design — but the test report will show a string-comparison diff rather than clearly saying "server returned 500". This makes debugging harder.A clearer alternative:
Suggested improvement
page.on("response", async (response) => { if (response.status() === 500) { - await expect(response.url()).toBe("This URL responded with a 500 status"); + throw new Error(`Server returned 500 for: ${response.url()}`); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts` around lines 21 - 27, The response handler in test.beforeEach currently uses expect(response.url()).toBe(...) which always fails and produces a confusing string-diff; instead, update the page.on("response") callback (the handler that checks response.status()) to surface a clear error when a 500 is seen — e.g., throw a descriptive Error (or call the test failure helper) with a message like "Server returned 500 for <url>" referencing response.url(), so replace the failing expect(...) in the response.status() === 500 branch with a throw/new Error that includes response.url().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts`:
- Around line 8-19: The suite currently risks parallel/nested ordering and
multiple beforeAll/afterAll runs because nested describes may use different
storageState values; add test.describe.configure({ mode: "serial" }) at the top
of the "File Upload - InfraCircuitContract" describe block to enforce serial
execution, and verify/adjust any nested test.use(...) that changes storageState
(or move branch creation into a global setup) so Playwright does not spawn
multiple workers and invoke test.beforeAll/test.afterAll per worker; check
Playwright config for fullyParallel/workers settings to ensure a single worker
or use a globalSetup to create/delete BRANCH_NAME instead of per-worker hooks.
---
Nitpick comments:
In `@frontend/app/tests/e2e/objects/file-upload/file-upload.spec.ts`:
- Around line 21-27: The response handler in test.beforeEach currently uses
expect(response.url()).toBe(...) which always fails and produces a confusing
string-diff; instead, update the page.on("response") callback (the handler that
checks response.status()) to surface a clear error when a 500 is seen — e.g.,
throw a descriptive Error (or call the test failure helper) with a message like
"Server returned 500 for <url>" referencing response.url(), so replace the
failing expect(...) in the response.status() === 500 branch with a throw/new
Error that includes response.url().
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.
Summary by CodeRabbit
Documentation
Tests