Fix e2e tests & playwright comment job#2392
Conversation
Summary of ChangesHello @wwwillchen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the stability and developer experience around the end-to-end testing suite. It addresses flakiness in existing tests by introducing more explicit steps and increases test timeouts. Additionally, it improves the utility of the Playwright summary generation script by providing more comprehensive commands for debugging and maintaining tests. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 Multi-Agent Code Review3 independent reviewers analyzed this PR. Found 1 issue with consensus. Summary
Issues to Address
🟢 Low Priority Issues (2 items - no consensus at MEDIUM+)
See inline comments for details. Generated by multi-agent consensus review (3 agents, 2+ agreement required) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| await po.goToHubAndSelectTemplate("Next.js Template"); | ||
|
|
||
| await po.selectChatMode("build"); |
There was a problem hiding this comment.
🟡 MEDIUM - Inconsistent test setup pattern (2/3 reviewers)
selectChatMode("build") is called after goToHubAndSelectTemplate, but the template may auto-select a chat mode. This pattern is also used in template-create-nextjs.spec.ts, so it's at least consistent across tests.
Considerations:
- The order may matter if the template auto-selects a different chat mode
- Consider whether selecting the chat mode before navigating would be more robust
- If this is intentional (overriding the template's default), a brief comment would clarify the intent
Flagged by 2/3 independent reviewers (1 MEDIUM, 1 LOW)
Greptile OverviewGreptile SummaryFixes E2E test flakiness and improves CI/CD tooling for Playwright test reporting. Key Changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant PO as Page Object
participant App as Dyad App
participant Preview as Preview Panel
Note over Test,Preview: Template Selection & Chat Mode Setup
Test->>PO: goToHubAndSelectTemplate("Next.js Template")
PO->>App: Select Next.js template
App-->>PO: Template loaded
Test->>PO: selectChatMode("build")
PO->>App: Set chat mode to "build"
App-->>PO: Chat mode updated
Note over Test,Preview: Test Execution Flow
Test->>PO: sendPrompt("tc=basic")
PO->>App: Send test command
App-->>PO: Command processed
Note over Test,Preview: Component Selection Test
Test->>PO: clickTogglePreviewPanel()
PO->>App: Toggle preview
App->>Preview: Show preview panel
Test->>PO: clickPreviewPickElement()
PO->>Preview: Enable element picker
Test->>Preview: Click element in iframe
Preview-->>App: Component selected
Note over Test,Preview: Version Upgrade Flow (select_component.spec.ts)
Test->>PO: clickAppUpgradeButton()
PO->>App: Trigger upgrade
App-->>PO: Upgrade complete, Version 2 visible
Test->>PO: clickRestart()
PO->>App: Restart application
App-->>PO: Restarted with new version
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves end-to-end test stability by adding a restart after app upgrades, explicitly setting chat mode, and increasing local test timeouts. It also enhances the Playwright summary script for easier debugging. However, a significant security vulnerability was found in the command generation logic of the summary script, where unescaped test titles could lead to command injection on a developer's machine. Addressing this vulnerability is crucial. Additionally, there's an opportunity to further simplify the command generation logic.
| // Escape special characters in testName for the grep pattern | ||
| const escapedTestName = testName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| return `npm run e2e e2e-tests/${specFile} -- --g="${escapedTestName}" --update-snapshots`; | ||
| return `npm run e2e e2e-tests/${specFile} -- -g "${escapedTestName}"`; |
There was a problem hiding this comment.
Critical Security Vulnerability: The generated shell command is vulnerable to command injection because testName and specFile are not properly escaped for the shell. An attacker could craft a malicious test title (e.g., containing backticks or double quotes) that executes arbitrary commands when a developer copy-pastes the suggested command. To fix this, you must escape shell-sensitive characters. Additionally, for improved maintainability and robustness, consider simplifying this by using the fullTitle directly in the grep pattern, which would also allow you to remove the parseTestTitle function.
| return `npm run e2e e2e-tests/${specFile} -- -g "${escapedTestName}"`; | |
| return "npm run e2e \"e2e-tests/" + specFile.replace(/["\x60\\$]/g, "\\$&") + "\" -- -g \"" + escapedTestName.replace(/["\x60]/g, "\\$&") + "\""; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
#skip-bb
Summary by cubic
Fixes failing Next.js e2e tests by selecting the correct chat mode and restarting after upgrades, and improves the Playwright PR comment with clearer run/update commands. Also increases local test timeout to reduce flaky failures.
Bug Fixes
New Features
Written for commit 5124cf2. Summary will update on new commits.