fix: restore lightspeed UI tests #2722
Conversation
|
Thank you @resoluteCoder, great job on:
Answers on your questions: test_vscode_widget: if we do not have Copilot subscription, I think we should remove multi-provider suggestion test for now and keep it in the git history if we need it later. @goneri, @omaciel, @ssbarnea we don't have the Copilot subscription for tests, right? New env vars for credentials: Looks like those tests do not tests the VS Code Ansible Plugin, but SaaS web page. We have stage/prod tests running every x hours on our side, I think those tests/credentials mentioning should be removed from the VS Code plugin test suite entirely. |
|
@hasys I agree with removing the copilot test and migrating to f1 shortcut. Should we do the f1 migration in another PR? Last I checked there were many other places that used the base run command. |
|
To confirm — when you say remove the tests/credentials, does that include removing the |
Up to you, depending on the amount of work required.
My understanding of those tests - only regular user actually used to test the VS Code Plugin functionality, other users were testing the Lightspeed WEB UI/access, not the plugin itself. That's why they weren't active (probably test cases were just copied together with other tests from SaaS testing). So the goal is to reflect the reality, since only regular user is used to test the functional of VS Code, remove all others users and their mentions. |
resoluteCoder
force-pushed
the
AAP-67210
branch
from
4630015 to
82682be
Compare
|
Alright sounds good. I'll make another PR for the f1 migration I also removed the 4 SaaS related web portal login tests (unsubed/no_wca variants), the trial test, and the multi-provider widget test along with their associated credentials and helpers. |
📝 WalkthroughWalkthroughVSCode UI test suite refactored to consolidate authentication approaches and streamline command execution patterns. Fixture scope modified, test utilities simplified with updated signatures, login/SSO flows updated, and obsolete trial-related tests removed across five test files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (3)
test/ui/utils/ui_utils.py (2)
322-339: Consider removing the unuseddevice_loginparameter.The
device_loginparameter is documented as "kept for compatibility" but is never used in the function body. Sincevscode_connectnow always uses the F1 command palette approach regardless of this parameter, consider removing it entirely in a follow-up to avoid confusion about its effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/utils/ui_utils.py` around lines 322 - 339, The function vscode_login has an unused parameter device_login which is documented as "kept for compatibility" but never referenced; remove the device_login parameter from the vscode_login signature and docstring, update any callers to stop passing that argument (or keep backward-compatible forwarding in callers), and ensure the flow still calls vscode_connect and sso_auth_flow and switches back to the main window as implemented; reference the vscode_login function and related vscode_connect and sso_auth_flow symbols when making the change.
718-718: Remove the unusedPOSITION_OF_ANSIBLE_EXPLAINconstant and related comment block.The constant at line 33 and associated comment (lines 721-725) are no longer needed since the F1 command palette approach doesn't rely on counting DOWN key presses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/utils/ui_utils.py` at line 718, Remove the now-unused POSITION_OF_ANSIBLE_EXPLAIN constant and its associated comment block (the explanatory lines about counting DOWN key presses) from the module; search for any references to POSITION_OF_ANSIBLE_EXPLAIN and delete or refactor them (likely near the vscode_run_command_f1 usage) so nothing depends on that constant, and run the tests to ensure no remaining references or docstrings break.test/ui/test_80_lightspeed.py (1)
117-125: Hardcoded username may be fragile.The check for
'lightspeed-test-user'at line 123 is hardcoded. If the test credentials change, this assertion will fail. Consider deriving this from theLIGHTSPEED_USERenvironment variable used elsewhere:💡 Suggested improvement
+import os + +LIGHTSPEED_USER = os.environ.get("LIGHTSPEED_USER", "") + # In test_vscode_lightspeed_explorer: wait_displayed( driver, - "//*[contains(normalize-space(.), 'lightspeed-test-user')]", + f"//*[contains(normalize-space(.), '{LIGHTSPEED_USER}')]", timeout=5, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ui/test_80_lightspeed.py` around lines 117 - 125, The test is asserting a hardcoded username string 'lightspeed-test-user' which is brittle; update the assertion in the block using wait_displayed (after accounts_button.click()) to derive the expected username from the LIGHTSPEED_USER environment variable instead of the literal. Specifically, change the locator argument passed to wait_displayed used in this test (the one that currently contains "lightspeed-test-user") to use os.environ.get('LIGHTSPEED_USER') (or the existing config accessor used elsewhere in tests), provide a sensible fallback if necessary, and ensure the string is normalized to match the existing XPath/normalize-space check so wait_displayed(driver, "//*[contains(normalize-space(.), '{expected_username}')]") continues to work; keep the rest of the flow (vscode_run_command_f1, accounts_button click) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ui/test_80_lightspeed.py`:
- Around line 117-125: The test is asserting a hardcoded username string
'lightspeed-test-user' which is brittle; update the assertion in the block using
wait_displayed (after accounts_button.click()) to derive the expected username
from the LIGHTSPEED_USER environment variable instead of the literal.
Specifically, change the locator argument passed to wait_displayed used in this
test (the one that currently contains "lightspeed-test-user") to use
os.environ.get('LIGHTSPEED_USER') (or the existing config accessor used
elsewhere in tests), provide a sensible fallback if necessary, and ensure the
string is normalized to match the existing XPath/normalize-space check so
wait_displayed(driver, "//*[contains(normalize-space(.),
'{expected_username}')]") continues to work; keep the rest of the flow
(vscode_run_command_f1, accounts_button click) unchanged.
In `@test/ui/utils/ui_utils.py`:
- Around line 322-339: The function vscode_login has an unused parameter
device_login which is documented as "kept for compatibility" but never
referenced; remove the device_login parameter from the vscode_login signature
and docstring, update any callers to stop passing that argument (or keep
backward-compatible forwarding in callers), and ensure the flow still calls
vscode_connect and sso_auth_flow and switches back to the main window as
implemented; reference the vscode_login function and related vscode_connect and
sso_auth_flow symbols when making the change.
- Line 718: Remove the now-unused POSITION_OF_ANSIBLE_EXPLAIN constant and its
associated comment block (the explanatory lines about counting DOWN key presses)
from the module; search for any references to POSITION_OF_ANSIBLE_EXPLAIN and
delete or refactor them (likely near the vscode_run_command_f1 usage) so nothing
depends on that constant, and run the tests to ensure no remaining references or
docstrings break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e56cdfe-5b39-4e09-852d-71256e66d15d
📒 Files selected for processing (5)
test/ui/fixtures/ui_fixtures.pytest/ui/test_80_lightspeed.pytest/ui/test_81_login.pytest/ui/test_82_lightspeed_trial.pytest/ui/utils/ui_utils.py
💤 Files with no reviewable changes (1)
- test/ui/test_82_lightspeed_trial.py
hasys
left a comment
hasys
left a comment
There was a problem hiding this comment.
Looks good to me now, thank you @resoluteCoder
|
Just a heads up on the |
github-project-automation
Bot
moved this from Review
to Done
in 🧰 devtools project board
## Summary - **Remove the entire Python/Selenium UI test suite** (`test/ui/`, `docker-compose.yml`, Selenium/Podman Python deps) — eliminates the fragile `selenium-vscode` container, brittle XPath selectors, and DOM mismatches between `code-server` and desktop VS Code. - **Add a WebDriverIO test suite** (`test/wdio/`, `wdio.conf.ts`) using `wdio-vscode-service`, which tests against real VS Code Electron. 30 tests across 7 spec files: smoke, commands, terminal, sidebar, webviews (devfile, devcontainer, welcome page, LLM provider settings), MCP server toggle, and Lightspeed (backed by an Express mock server with injected auth session). - **Dedicated CI runner** — WDIO tests run on their own `test (linux-wdio)` matrix entry, independent of unit/e2e. No more cascading failures from unrelated e2e issues blocking UI tests. - **Update CI** (`ci.yaml`, `Taskfile.yml`) to run `task wdio` under `xvfb-run` instead of the Selenium container, removing the Podman dependency from Linux CI. - **Add onboarding docs** — Cursor skill (`.cursor/skills/wdio-testing/`) with architecture, patterns, and gotchas; updated `test/README.md` and `docs/development/test_code.md`. ## Why All 29 Selenium tests on `main` are currently **skipped** — zero tests actually execute: | File | Tests | Status | Skip Reason | |------|-------|--------|-------------| | `test_00_commands.py` | 2 | All skipped | Flaky on CI - extension activation timing | | `test_01_dev_webviews.py` | 2 | All skipped | Flaky on CI - extension activation timing | | `test_02_welcome_webviews.py` | 3 | All skipped | Flaky on CI - extension activation timing | | `test_03_llm_provider_webview.py` | 4 | All skipped | Flaky on CI - extension activation timing | | `test_50_mcp_server.py` | 4 | All skipped | Flaky on CI - extension activation timing | | `test_80_lightspeed.py` | 5 | All skipped | AAP-67210 | | `test_81_login.py` | 8 | All skipped | AAP-67210 | | `test_82_lightspeed_trial.py` | 1 | All skipped | AAP-67210 | | **Total** | **29** | **0 running** | | The Selenium suite had a **100% CI failure rate** for the last two weeks on `main` (W15-W16) before the tests were skipped, and an accelerating failure rate before that (5% → 16% → 38% → 100% over 8 weeks). Root causes: `code-server` DOM divergence from real VS Code, fragile XPath selectors, deeply nested iframe traversal, and container startup timing. See [full analysis](https://gist.github.com/cidrblock/eaa433824950742554bee78d217e3772). WDIO with `wdio-vscode-service` provides stable Page Objects against the real VS Code Electron shell, `browser.executeWorkbench()` for direct access to the VS Code API, and zero container overhead. ## AAP-67210 Coverage The 14 Lightspeed/SSO tests tracked by [AAP-67210](https://redhat.atlassian.net/browse/AAP-67210) were all skipped in Selenium. WDIO restores coverage for 5 of them using a mock server approach, and adds 2 new tests (error/timeout handling) that never existed: | AAP-67210 Selenium Test | WDIO Status | WDIO Test | |---|---|---| | `test_vscode_widget` | **Covered** | `should have Lightspeed commands registered` | | `test_vscode_playbook_explanation` | **Covered** | `should open playbook explanation webview` | | `test_vscode_playbook_generation` | **Covered** | `should open playbook generation webview` | | `test_vscode_role_generation` | **Covered** | `should open role generation webview` | | `test_vscode_lightspeed_explorer` | **Partial** | Explorer view covered via sidebar tests | | `test_vscode_trial_button` | Deferred | Excluded from initial PR | | `test_unsubed_login` | N/A | SSO flow — tests RH infra, not extension | | `test_unsubed_admin_login` | N/A | SSO flow | | `test_no_wca_user_login` | N/A | SSO flow | | `test_no_wca_admin_login` | N/A | SSO flow | | `test_login_page` | N/A | SSO flow | | `test_sso_auth_flow` | N/A | SSO flow | | `test_admin_portal_error` | N/A | SSO flow | | `test_vscode_rhsso_auth_flow` | N/A | SSO flow | The 8 SSO login tests exercise Red Hat SSO infrastructure (external OAuth pages), not extension code. They require live SSO credentials and a running Lightspeed backend, which is why they were never functional in CI. ## CI architecture ``` Before: test (linux) → task unit → task e2e → task ui (selenium, blocked by e2e) After: test (linux) → task unit → task e2e test (linux-wdio) → task build → task wdio ← independent runner ``` The WDIO job skips `task package` (no `.vsix` needed) and runs only `task build` → `task wdio`. Either job can be re-run independently. ## Test coverage comparison | Area | Selenium (before) | WDIO (after) | |---|---|---| | Smoke / activation | implicit | 5 explicit tests | | Commands | 1 (create playbook) | 1 (create playbook) | | Terminal | 1 (flaky, DOM scraping) | 2 (execSync + API) | | Webviews | 4 (devfile, devcontainer, welcome, LLM) | 9 (deeper assertions) | | MCP Server | 2 | 4 (enable/disable/settings/list) | | Sidebar | 0 | 3 (view, welcome link, settings) | | Lightspeed | 0 (all skipped/SSO-gated) | 6 (mock server: explain, generate, role, commands, errors, timeout) | | **Total** | **0 active (29 skipped)** | **30 (all passing)** | ## Extension source changes - `src/extension.ts`: registers `ansible.lightspeed.mockSession` command (gated to `ExtensionMode.Development`/`Test` only — not available in production) - `src/features/lightspeed/lightSpeedOAuthProvider.ts`: adds `setMockSession()` to inject fake auth sessions ## Test plan - [x] `task wdio` passes locally under `xvfb-run` - [x] CI `test (linux-wdio)` job passes - [x] `task lint` passes - [x] `task unit` / `task e2e` unaffected on `test (linux)` - [x] No Selenium/Podman references remain in CI or Taskfile Closes: ansible#2722 Closes: ansible#2743 Closes: ansible#2720 Closes: ansible#2752 related: AAP-67210
3 participants
UI Test Changes — AAP-67210
Overview
Restored 13 of 14 skipped lightspeed UI tests. 1 remains skipped (
test_vscode_widget— requires Copilot in test container).Common changes across all tests
@pytest.mark.skip(reason="See https://redhat.atlassian.net/browse/AAP-67210")close_editorsfixture from web portal tests (test_81) — they never open VSCode editors, so the fixture added ~2 min overhead navigating to code-server unnecessarily.ui_fixtures.py
scope="session"toscope="module"— isolates SSO cookies between test files. Without this, test_81 (web portal login tests) fails because test_80 leaves SSO cookies from the VSCode device login flow.test_80_lightspeed.py
close_editors.close_editors.close_editors.close_editors. Updated for redesigned sidebar:vscode-buttonelements and "Logged in as:" text in a webview.//a[@aria-label='Accounts'], verify "lightspeed-test-user" appears) to replace removed "Logged in as:" assertion.device_login=True.test_81_login.py
close_editors."Log in with Ansible Automation Platform"to"Log in with Red Hat"to match current staging UI.device_login=True. Removed dead commented-out code.test_82_lightspeed_trial.py
close_editors. Addeddevice_login=Truetovscode_login()call.ui_utils.py
user_menuparam and sidebar Connect button branch — both relied on UI that no longer exists. Only OAuth2 Device Flow path remains. Changeddevice_logindefault toTrue.device_logindefault toTrue.action-item command-center-center), which broke in newer VSCode versions. Also fixed palette matching — was matching wrong commands because Enter fired before filtering completed. Now finds and clicks the correct item in the dropdown list. Falls back to Enter if not found.vscode_run_command_f1.vscode_run_commandtovscode_run_command_f1.no_sub,no_wca,admin_loginflags. Previously always usedLIGHTSPEED_USER/LIGHTSPEED_PASSWORD. Now uses env vars:NO_SUB_USER,NO_SUB_PASSWORD,NO_SUB_ADMIN,NO_WCA_USER,NO_WCA_PASSWORD,NO_WCA_ADMIN.Testing
Ran all 13 restored tests 5 consecutive times locally — all passed every run. This was done to verify stability since the original tests were marked as broken/skipped and flakiness was a concern.
Open discussion
vscode_run_command_f1uses the F1 shortcut instead of clicking the command center UI element. This was needed because the command center XPath broke across VSCode versions. F1 is a more future-proof approach since it's a stable keyboard shortcut. Worth discussing whether to migratevscode_run_commandcallers over to the F1 approach or keep both.sso_auth_flow()acceptedno_sub,no_wca, andadmin_loginparams but ignored them — it always usedLIGHTSPEED_USER/LIGHTSPEED_PASSWORD. Since these tests were always skipped, this was never caught. We added separate env vars (NO_SUB_*,NO_WCA_*) so each test scenario uses the correct account. CI currently only providesLIGHTSPEED_USER,LIGHTSPEED_PASSWORD, andLIGHTSPEED_API_ENDPOINT— the new vars need to be added to CI secrets.This PR restores 13 previously skipped Lightspeed UI tests by removing pytest skip decorators and updating test utilities. Changes include reducing browser_setup scope from session to module for SSO cookie isolation, defaulting device login for VS Code flows, consolidating to F1-based command palette interactions, and removing SaaS-related tests.
browser_setupfixture scope fromsessiontomoduleto isolate SSO cookies between test modules@pytest.mark.skipfromtest_sso_auth_flow,test_admin_portal_error, andtest_vscode_rhsso_auth_flowin test_81_login.pytest_unsubed_login,test_unsubed_admin_login,test_no_wca_user_login,test_no_wca_admin_login)vscode_login()to defaultdevice_login=Trueand removed**kwargspassthroughvscode_connect()to always use F1-based command palette path and defaultdevice_login=Truesso_auth_flow()by removing optionaladmin_login,no_wca, andno_subparametersvscode_run_command_f1()helper for F1-based command palette interactions with fallback supportvscode_run_command()calls withvscode_run_command_f1()in test_80_lightspeed.py and ui_utils.pyredhat_logout()to handle SSO logout confirmation clickvscode_trial_button()helper functiontest_vscode_lightspeed_explorerfor redesigned Lightspeed sidebar buttonstest_vscode_lightspeed_explorerrelated: AAP-67210