🧪 Add a non_agent_mode e2e test #925
Conversation
ibolton336
force-pushed
the
non-agentic-test
branch
from
512a8b5 to
4ed876d
Compare
WalkthroughAdds a new end-to-end test file tests/e2e/tests/non_agent_flow_coolstore.test.ts that exercises an offline (non-agent) Coolstore workflow via the VSCode UI test harness: profile setup, generative AI configuration, server start, LLM cache population, analysis run, locating a JMS-related violation, requesting a solution, waiting for solution readiness, optionally accepting changes, and capturing screenshots. Updates tests/e2e/tests/agent_flow_coolstore.test.ts to call a new verification step and introduces verifyAnalysisViewCleanState in tests/e2e/utilities/utils.ts. No public APIs or exported interfaces were changed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (3)
26-30: Refine error handling to avoid masking legitimate failures.The generic
catchblock swallows all errors fromdeleteProfile, not just "profile doesn't exist" scenarios. This could hide legitimate failures such as permission issues or network problems, making debugging harder.Consider catching and checking for the specific error condition:
- try { - await vscodeApp.deleteProfile(profileName); - } catch { - console.log(`An existing profile probably doesn't exist, creating a new one`); - } + try { + await vscodeApp.deleteProfile(profileName); + } catch (error) { + console.log(`Profile deletion failed (likely doesn't exist): ${error}`); + // Continue to create profile + }Or verify whether the profile exists before attempting deletion.
100-126: Consider using Playwright's built-in wait mechanisms.The custom polling loop reinvents functionality that Playwright provides through
waitForwith custom conditions orPromise.race. While the logic is sound, using framework-native mechanisms improves reliability and maintainability.Consider refactoring to use Playwright's wait utilities:
- // Wait for the solution to be generated and presented - const acceptChangesLocator = resolutionView.locator( - 'button[aria-label="Accept all changes"]' - ); - - // Wait for either the accept changes button or some indication that solutions are ready - let solutionReady = false; - let maxWaitTime = 60; // 60 seconds max wait - - while (!solutionReady && maxWaitTime > 0) { - const acceptButtonVisible = (await acceptChangesLocator.count()) > 0; - const solutionText = (await resolutionView.getByText('Solution').count()) > 0; - const codeChanges = (await resolutionView.locator('.monaco-editor').count()) > 0; - - if (acceptButtonVisible || solutionText || codeChanges) { - solutionReady = true; - console.log('Solution appears to be ready'); - } else { - console.log(`Waiting for solution to be ready... ${maxWaitTime} seconds remaining`); - await vscodeApp.getWindow().waitForTimeout(1000); - maxWaitTime--; - } - } - - if (!solutionReady) { - throw new Error('Solution was not ready within the expected time frame'); - } + // Wait for any solution indicator to appear + await Promise.race([ + resolutionView.locator('button[aria-label="Accept all changes"]').waitFor({ timeout: 60000 }), + resolutionView.getByText('Solution').waitFor({ timeout: 60000 }), + resolutionView.locator('.monaco-editor').waitFor({ timeout: 60000 }), + ]).then(() => console.log('Solution appears to be ready')) + .catch(() => { + throw new Error('Solution was not ready within the expected time frame'); + }); + + const acceptChangesLocator = resolutionView.locator( + 'button[aria-label="Accept all changes"]' + );
137-165: Add verification after accepting changes.After clicking "Accept all changes", the test waits briefly and takes a screenshot but doesn't verify that changes were actually applied. The test could pass even if the solution acceptance failed silently.
Consider adding verification logic after accepting changes, such as:
// After clicking accept changes button if ((await acceptChangesLocator.count()) > 0) { await acceptChangesLocator.click(); console.log('Accept all changes button clicked'); await vscodeApp.getWindow().screenshot({ path: pathlib.join( SCREENSHOTS_FOLDER, 'non_agentic_flow_coolstore', `${config.model.replace(/[.:]/g, '-')}`, `changes-accepted.png` ), }); // Verify changes were applied (example - adjust based on actual success indicators) const successIndicator = resolutionView.getByText('Changes applied successfully'); await expect(successIndicator).toBeVisible({ timeout: 30000 }); }Adapt the verification logic based on the actual UI success indicators in your application.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (6)
tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_GPT4O_PROVIDER(23-34)tests/e2e/pages/vscode.page.ts (1)
VSCode(29-690)tests/e2e/utilities/utils.ts (1)
getRepoName(75-84)tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)tests/e2e/enums/configuration-options.enum.ts (2)
kaiCacheDir(6-6)kaiDemoMode(7-7)tests/e2e/enums/views.enum.ts (1)
KAIViews(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (3)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (3)
1-16: LGTM!The imports are well-organized, and the configuration choices (single provider, hardcoded profile name) are appropriate for a cached, offline test scenario.
46-82: LGTM!The test setup and analysis flow is well-structured:
- VSCode settings are properly configured for demo mode and cache directory
- Analysis completion is waited for with appropriate timeout
- Agent mode toggling is defensive (checks current state before action)
- Violation search and fix button interaction follow expected patterns
The 60-minute timeout is extremely long but may be justified for complex E2E UI automation scenarios.
176-181: LGTM!The cleanup logic is appropriate:
- Conditional LLM cache update based on environment variable
- Proper VSCode cleanup to avoid resource leaks
WalkthroughAdds a new end-to-end test suite for the Coolstore app running in non-agent mode with offline/cached LLM data. The test initializes a VSCode-based harness, prepares a cached profile, launches the server with LLM cache, and captures screenshots across stages. It ensures analysis completion, toggles agent mode off if needed, locates a JMS violation, triggers a fix, opens the resolution view, waits for a solution, and conditionally accepts all changes. Hooks handle setup/teardown, per-test logging/screenshots, cache updates, and error handling (e.g., profile deletion fallback). The suite iterates over providers/models (currently one) and produces provider-scoped artifacts. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (1)
38-43: Centralize sanitized names for deterministic screenshot paths.Right now
test.info().title.replace(' ', '-')only touches the first space, so the file names still contain whitespace. The same path builds also repeatconfig.model.replace(/[.:]/g, '-')several times. Precomputing sanitized names (and the provider subdirectory) once keeps the artifacts consistent and removes the duplication.@@ - test.beforeEach(async () => { - const testName = test.info().title.replace(' ', '-'); + const sanitizedModelName = config.model.replace(/[.:]/g, '-'); + const providerScreenshotDir = pathlib.join( + SCREENSHOTS_FOLDER, + 'non_agentic_flow_coolstore', + sanitizedModelName + ); + + test.beforeEach(async () => { + const testName = test.info().title.replace(/\s+/g, '-'); @@ - path: `${SCREENSHOTS_FOLDER}/before-${testName}-${config.model.replace(/[.:]/g, '-')}.png`, + path: `${SCREENSHOTS_FOLDER}/before-${testName}-${sanitizedModelName}.png`, @@ - path: pathlib.join( - SCREENSHOTS_FOLDER, - 'non_agentic_flow_coolstore', - `${config.model.replace(/[.:]/g, '-')}`, - `resolution-view-before-non-agent-flow.png` - ), + path: pathlib.join( + providerScreenshotDir, + `resolution-view-before-non-agent-flow.png` + ), @@ - path: pathlib.join( - SCREENSHOTS_FOLDER, - 'non_agentic_flow_coolstore', - `${config.model.replace(/[.:]/g, '-')}`, - `solution-ready.png` - ), + path: pathlib.join(providerScreenshotDir, `solution-ready.png`), @@ - path: pathlib.join( - SCREENSHOTS_FOLDER, - 'non_agentic_flow_coolstore', - `${config.model.replace(/[.:]/g, '-')}`, - `changes-accepted.png` - ), + path: pathlib.join(providerScreenshotDir, `changes-accepted.png`), @@ - path: pathlib.join( - SCREENSHOTS_FOLDER, - 'non_agentic_flow_coolstore', - `${config.model.replace(/[.:]/g, '-')}`, - `final-state.png` - ), + path: pathlib.join(providerScreenshotDir, `final-state.png`),Also applies to: 87-135, 142-149, 156-163
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (6)
tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_GPT4O_PROVIDER(23-34)tests/e2e/pages/vscode.page.ts (1)
VSCode(29-690)tests/e2e/utilities/utils.ts (1)
getRepoName(75-84)tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)tests/e2e/enums/configuration-options.enum.ts (2)
kaiCacheDir(6-6)kaiDemoMode(7-7)tests/e2e/enums/views.enum.ts (1)
KAIViews(3-7)
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
ibolton336
force-pushed
the
non-agentic-test
branch
from
27bd2e5 to
9c08619
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (2)
38-44: Replace only the first space.As flagged in previous reviews,
replace(' ', '-')only replaces the first space occurrence. For a test title like "Fix JMS Topic issue with agent mode disabled (offline)", this produces inconsistent filenames with remaining spaces.Apply this fix:
- const testName = test.info().title.replace(' ', '-'); + const testName = test.info().title.replace(/\s+/g, '-');
181-187: Replace only the first space.Same issue as line 39:
replace(' ', '-')only replaces the first space occurrence in the test name, resulting in inconsistent screenshot filenames.Apply this fix:
- const testName = test.info().title.replace(' ', '-'); + const testName = test.info().title.replace(/\s+/g, '-');
🧹 Nitpick comments (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (1)
97-127: Consider using Playwright's built-in waitFor.The manual polling loop works but could be simplified using Playwright's
waitForwith a callback orPromise.racepattern. However, the current approach is functional and explicit about what it's waiting for.Example using
Promise.race:await Promise.race([ acceptChangesLocator.waitFor({ state: 'visible', timeout: 60000 }), resolutionView.getByText('Solution').waitFor({ timeout: 60000 }), resolutionView.locator('.monaco-editor').waitFor({ timeout: 60000 }) ]).catch(() => { throw new Error('Solution was not ready within the expected time frame'); }); console.log('Solution appears to be ready');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/tests/agent_flow_coolstore.test.ts(2 hunks)tests/e2e/tests/non_agent_flow_coolstore.test.ts(1 hunks)tests/e2e/utilities/utils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T16:16:56.005Z
Learnt from: abrugaro
PR: konveyor/editor-extensions#657
File: tests/global.setup.ts:22-27
Timestamp: 2025-08-05T16:16:56.005Z
Learning: In the Playwright test framework for konveyor/editor-extensions, the global setup in tests/global.setup.ts is designed to handle only essential initialization like extension installation verification. Individual tests are responsible for configuring GenAI when needed, rather than doing it globally. The openAnalysisView() method is used as a fast verification mechanism to ensure the extension was successfully installed, which is more efficient than configuring GenAI globally for all tests.
Applied to files:
tests/e2e/utilities/utils.ts
🧬 Code graph analysis (3)
tests/e2e/tests/agent_flow_coolstore.test.ts (2)
tests/e2e/utilities/utils.ts (1)
verifyAnalysisViewCleanState(131-163)tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (6)
tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_GPT4O_PROVIDER(23-34)tests/e2e/pages/vscode.page.ts (1)
VSCode(29-690)tests/e2e/utilities/utils.ts (2)
getRepoName(79-88)verifyAnalysisViewCleanState(131-163)tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)tests/e2e/enums/configuration-options.enum.ts (2)
kaiCacheDir(6-6)kaiDemoMode(7-7)tests/e2e/enums/views.enum.ts (1)
KAIViews(3-7)
tests/e2e/utilities/utils.ts (2)
tests/e2e/pages/vscode.page.ts (1)
VSCode(29-690)tests/e2e/enums/views.enum.ts (1)
KAIViews(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
- GitHub Check: Build (macos)
🔇 Additional comments (9)
tests/e2e/utilities/utils.ts (2)
8-11: LGTM!The new imports are necessary for the
verifyAnalysisViewCleanStatefunction and are correctly structured with proper type imports.
122-163: LGTM!The
verifyAnalysisViewCleanStatefunction is well-structured for e2e testing:
- Navigates to analysis view and retrieves the frame
- Waits for loading indicators (backdrop, spinner, waiting text) to disappear with appropriate 30-second timeouts
- Verifies the analysis table is visible
- Captures a final screenshot with logging
The use of PatternFly v6 OUIA attributes (
data-ouia-component-type) provides stable test selectors.tests/e2e/tests/agent_flow_coolstore.test.ts (2)
9-9: LGTM!Import of the new verification utility is correct.
144-155: LGTM!The placement of
verifyAnalysisViewCleanStateafter the agent flow loop is appropriate. It verifies the analysis view returns to a clean, interactive state after all agent actions complete, with a well-structured screenshot path and descriptive log prefix.tests/e2e/tests/non_agent_flow_coolstore.test.ts (5)
1-17: LGTM!Imports and configuration are appropriate. The single-provider list with a hardcoded profile name ensures cache consistency and reproducible test runs.
19-36: LGTM!The
beforeAllsetup correctly initializes the test environment: launches VSCode, manages profiles, configures GenAI, starts the server, and ensures LLM cache is available. This mirrors the agent flow test setup, maintaining consistency.
46-95: LGTM!The test setup correctly:
- Configures demo mode and cache directory
- Runs analysis and waits for completion
- Defensively ensures agent mode is disabled
- Locates the JMS violation and clicks the fix button
- Captures pre-resolution screenshot
The agent mode check (lines 68-73) is good defensive programming.
129-179: LGTM!The solution acceptance flow is well-structured:
- Captures screenshot when solution is ready
- Conditionally clicks "Accept all changes" if available
- Takes progress screenshots
- Verifies final state using
verifyAnalysisViewCleanState- Logs completion
The use of the shared verification utility maintains consistency with the agent flow test.
189-194: LGTM!The
afterAllcleanup correctly handles optional cache updates and closes VSCode, mirroring the agent flow test cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (2)
39-39: Replace all spaces in test name for consistent screenshot filenames.This issue was already flagged in previous reviews:
replace(' ', '-')only replaces the first space occurrence. Usereplace(/\s+/g, '-')to replace all whitespace.Apply this fix:
- const testName = test.info().title.replace(' ', '-'); + const testName = test.info().title.replace(/\s+/g, '-');
182-182: Replace all spaces in test name for consistent screenshot filenames.Same issue as line 39:
replace(' ', '-')only replaces the first space occurrence. Usereplace(/\s+/g, '-')to replace all whitespace.Apply this fix:
- const testName = test.info().title.replace(' ', '-'); + const testName = test.info().title.replace(/\s+/g, '-');
🧹 Nitpick comments (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (1)
110-123: Consider using Playwright's built-in wait utilities for improved readability.The polling loop is functional but could be replaced with Playwright's
waitForwith a custom condition, which handles retries and timeouts more elegantly.Example refactor:
await expect(async () => { const acceptButtonVisible = (await acceptChangesLocator.count()) > 0; const solutionText = (await resolutionView.getByText('Solution').count()) > 0; const codeChanges = (await resolutionView.locator('.monaco-editor').count()) > 0; expect(acceptButtonVisible || solutionText || codeChanges).toBeTruthy(); }).toPass({ timeout: 60000 }); console.log('Solution appears to be ready');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/tests/agent_flow_coolstore.test.ts(2 hunks)tests/e2e/tests/non_agent_flow_coolstore.test.ts(1 hunks)tests/e2e/utilities/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/utilities/utils.ts
- tests/e2e/tests/agent_flow_coolstore.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (6)
tests/e2e/fixtures/provider-configs.fixture.ts (1)
OPENAI_GPT4O_PROVIDER(23-34)tests/e2e/pages/vscode.page.ts (1)
VSCode(29-690)tests/e2e/utilities/utils.ts (2)
getRepoName(78-87)verifyAnalysisViewCleanState(130-165)tests/e2e/utilities/consts.ts (1)
SCREENSHOTS_FOLDER(2-2)tests/e2e/enums/configuration-options.enum.ts (2)
kaiCacheDir(6-6)kaiDemoMode(7-7)tests/e2e/enums/views.enum.ts (1)
KAIViews(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build (macos)
- GitHub Check: Build (windows)
- GitHub Check: Build (linux)
🔇 Additional comments (6)
tests/e2e/tests/non_agent_flow_coolstore.test.ts (6)
1-18: LGTM! Clean imports and well-documented configuration.The imports are appropriate, and the hardcoded profile name is clearly documented for cache consistency. The provider array structure allows for future expansion.
22-36: LGTM! Robust setup with defensive error handling.The beforeAll hook properly initializes the test environment with appropriate timeouts. The try-catch around profile deletion prevents setup failures when the profile doesn't exist.
47-61: LGTM! Proper test configuration and analysis execution.The test correctly configures demo mode and cache directory, then executes the analysis with appropriate timeout handling.
64-73: LGTM! Defensive agent mode verification.The defensive check ensures agent mode is disabled, logging the outcome whether it needed to be toggled or was already in the correct state.
167-176: LGTM! Proper use of verification utility.The test correctly calls
verifyAnalysisViewCleanStateto ensure the analysis view is in a clean, interactive state after the non-agent flow completes.
189-194: LGTM! Proper cleanup and conditional cache management.The afterAll hook correctly handles cleanup, with conditional LLM cache updates for development workflows and proper VSCode closure.
2 participants
Summary by CodeRabbit