From 371bb9f27982182e9514f7eeec86aaf630484498 Mon Sep 17 00:00:00 2001 From: Lukas Masuch Date: Tue, 13 Aug 2024 00:10:29 +0200 Subject: [PATCH] Migrate to test-state attribute to capture e2e script state (#9131) ## Describe your changes The e2e playwright tests currently rely on the status widget to check for the running state. This PR changes that to use the internal `data-teststate` on `stApp` instead which is more accurate. Also, we might soon change the behaviour of the status widget. ## Testing Plan - This only involves changes to our e2e tests. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --- e2e_playwright/app_hotkeys.py | 8 +++ e2e_playwright/app_hotkeys_test.py | 16 ++++-- e2e_playwright/conftest.py | 14 +++++- e2e_playwright/shared/app_utils.py | 30 ++++++++++- frontend/app/src/App.test.tsx | 75 ++++++++++++++++++++++++++++ frontend/app/src/App.tsx | 3 +- frontend/cypress/support/commands.js | 6 +-- frontend/lib/src/ScriptRunState.ts | 2 +- 8 files changed, 142 insertions(+), 12 deletions(-) diff --git a/e2e_playwright/app_hotkeys.py b/e2e_playwright/app_hotkeys.py index c9302511e779..a71c37a8b347 100644 --- a/e2e_playwright/app_hotkeys.py +++ b/e2e_playwright/app_hotkeys.py @@ -12,6 +12,14 @@ # See the License for the specific language governing permissions and # limitations under the License. + import streamlit as st +if "counter" not in st.session_state: + st.session_state.counter = 0 + +st.session_state.counter += 1 + st.text_input("text_input") + +st.write("Script runs:", st.session_state.counter) diff --git a/e2e_playwright/app_hotkeys_test.py b/e2e_playwright/app_hotkeys_test.py index 7e0d38a1ce83..bbf8e8e0a649 100644 --- a/e2e_playwright/app_hotkeys_test.py +++ b/e2e_playwright/app_hotkeys_test.py @@ -14,6 +14,11 @@ from playwright.sync_api import Page, expect +from e2e_playwright.conftest import wait_for_app_run +from e2e_playwright.shared.app_utils import ( + expect_prefixed_markdown, +) + def test_shows_clear_cache_dialog_when_c_is_pressed(app: Page): app.keyboard.type("c") @@ -40,12 +45,15 @@ def test_does_not_clear_cache_dialog_when_c_is_pressed_inside_text_input(app: Pa def test_reruns_when_r_is_pressed(app: Page): + expect_prefixed_markdown(app, "Script runs:", "1", exact_match=False) app.keyboard.type("r") - expect(app.get_by_test_id("stStatusWidget")).to_be_visible() + wait_for_app_run(app) + expect_prefixed_markdown(app, "Script runs:", "2", exact_match=False) -def test_does_not_clear_cache_dialog_when_r_is_pressed_inside_text_input( +def test_does_not_rerun_when_r_is_pressed_inside_text_input( app: Page, ): - app.get_by_test_id("stTextInput").press("r") - expect(app.get_by_test_id("stStatusWidget")).not_to_be_visible() + app.get_by_test_id("stTextInput").locator("input").press("r") + wait_for_app_run(app) + expect_prefixed_markdown(app, "Script runs:", "1", exact_match=False) diff --git a/e2e_playwright/conftest.py b/e2e_playwright/conftest.py index 4e391546b846..ceed0b4ab9de 100644 --- a/e2e_playwright/conftest.py +++ b/e2e_playwright/conftest.py @@ -612,9 +612,19 @@ def wait_for_app_run(page: Page, wait_delay: int = 100): """Wait for the given page to finish running.""" # Add a little timeout to wait for eventual debounce timeouts used in some widgets. page.wait_for_timeout(155) - + # Make sure that the websocket connection is established. + page.wait_for_selector( + "[data-testid='stApp'][data-test-connection-state='CONNECTED']", + timeout=20000, + state="attached", + ) + # Wait until we know the script has started. We determine this by checking + # whether the app is in notRunning state. (The data-test-connection-state attribute + # goes through the sequence "initial" -> "running" -> "notRunning"). page.wait_for_selector( - "[data-testid='stStatusWidget']", timeout=20000, state="detached" + "[data-testid='stApp'][data-test-script-state='notRunning']", + timeout=20000, + state="attached", ) if wait_delay > 0: diff --git a/e2e_playwright/shared/app_utils.py b/e2e_playwright/shared/app_utils.py index f82a0d8346cb..354f9837d958 100644 --- a/e2e_playwright/shared/app_utils.py +++ b/e2e_playwright/shared/app_utils.py @@ -16,7 +16,7 @@ import platform import re -from typing import Pattern +from typing import Literal, Pattern from playwright.sync_api import Locator, Page, expect @@ -410,3 +410,31 @@ def expect_help_tooltip( position={"x": 0, "y": 0}, no_wait_after=True, force=True ) expect(tooltip_content).not_to_be_attached() + + +def expect_script_state( + page: Page, + state: Literal[ + "initial", + "running", + "notRunning", + "rerunRequested", + "stopRequested", + "compilationError", + ], +) -> None: + """Expect the app to be in a specific script state. + + Parameters + ---------- + page : Page + The page to search for the script state. + + state : + The expected script state. + """ + page.wait_for_selector( + f"[data-testid='stApp'][data-test-script-state='{state}']", + timeout=10000, + state="attached", + ) diff --git a/frontend/app/src/App.test.tsx b/frontend/app/src/App.test.tsx index 7dc21f4a0d31..9ac4ec1bb52b 100644 --- a/frontend/app/src/App.test.tsx +++ b/frontend/app/src/App.test.tsx @@ -343,6 +343,48 @@ describe("App", () => { expect(getMockConnectionManager().disconnect).toHaveBeenCalled() }) + it("correctly sets the data-test-script-state attribute", async () => { + renderApp(getProps()) + + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-script-state", + "initial" + ) + + sendForwardMessage("newSession", NEW_SESSION_JSON) + + sendForwardMessage("sessionStatusChanged", { + runOnSave: false, + scriptIsRunning: true, + }) + + await waitFor(() => { + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-script-state", + ScriptRunState.RUNNING + ) + }) + + sendForwardMessage("sessionStatusChanged", { + runOnSave: false, + scriptIsRunning: false, + }) + + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-script-state", + ScriptRunState.NOT_RUNNING + ) + + sendForwardMessage("sessionEvent", { + type: "scriptCompilationException", + }) + + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-script-state", + ScriptRunState.COMPILATION_ERROR + ) + }) + describe("streamlit server version changes", () => { let prevWindowLocation: Location @@ -1976,6 +2018,39 @@ describe("App", () => { }) }) + it("Correctly sets the data-test-connection-state attribute", () => { + renderApp(getProps()) + + const connectionManager = getMockConnectionManager(false) + + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-connection-state", + ConnectionState.INITIAL + ) + + act(() => + // @ts-expect-error - connectionManager.props is private + connectionManager.props.connectionStateChanged( + ConnectionState.CONNECTED + ) + ) + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-connection-state", + ConnectionState.CONNECTED + ) + + act(() => + // @ts-expect-error - connectionManager.props is private + connectionManager.props.connectionStateChanged( + ConnectionState.PINGING_SERVER + ) + ) + expect(screen.getByTestId("stApp")).toHaveAttribute( + "data-test-connection-state", + ConnectionState.PINGING_SERVER + ) + }) + it("Sets attemptingToReconnect to false if DISCONNECTED_FOREVER", () => { renderApp(getProps()) diff --git a/frontend/app/src/App.tsx b/frontend/app/src/App.tsx index c842098f95ac..a9220be4b3a9 100644 --- a/frontend/app/src/App.tsx +++ b/frontend/app/src/App.tsx @@ -1908,11 +1908,12 @@ export class App extends PureComponent { {/* The tabindex below is required for testing. */}
diff --git a/frontend/cypress/support/commands.js b/frontend/cypress/support/commands.js index e866f70a8212..af10ed197d58 100644 --- a/frontend/cypress/support/commands.js +++ b/frontend/cypress/support/commands.js @@ -168,9 +168,9 @@ Cypress.Commands.add("loadApp", (appUrl, timeout) => { Cypress.Commands.add("waitForScriptFinish", (timeout = 20000) => { // Wait until we know the script has started. We determine this by checking - // whether the app is in notRunning state. (The data-teststate attribute goes - // through the sequence "initial" -> "running" -> "notRunning") - cy.get("[data-testid='stApp'][data-teststate='notRunning']", { + // whether the app is in notRunning state. (The data-test-connection-state attribute + // goes through the sequence "initial" -> "running" -> "notRunning") + cy.get("[data-testid='stApp'][data-test-script-state='notRunning']", { timeout, }).should("exist") }) diff --git a/frontend/lib/src/ScriptRunState.ts b/frontend/lib/src/ScriptRunState.ts index 64109e934ff1..d8d29406a43c 100644 --- a/frontend/lib/src/ScriptRunState.ts +++ b/frontend/lib/src/ScriptRunState.ts @@ -15,7 +15,7 @@ */ export enum ScriptRunState { - // TODO: Add INITIAL state here and clean up data-teststate in App.tsx. + // TODO: Add INITIAL state here and clean up data-test-script-state in App.tsx. // But before we do this, we need to make sure Snowflake hosts that use this // state will not break. And that's a bigger project... //INITIAL = "initial",