Copilot commented on code in PR #63516:
URL: https://github.com/apache/airflow/pull/63516#discussion_r3066477596
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -309,13 +287,10 @@ export class ConnectionsPage extends BasePage {
await expect
.poll(
async () => {
- const count1 = await this.page.locator("tbody tr").count();
-
- await this.page.evaluate(() => new Promise((r) => setTimeout(r,
200)));
- const count2 = await this.page.locator("tbody tr").count();
+ const count = await this.page.locator("tbody tr").count();
- if (count1 === count2 && count1 > 0) {
- stableRowCount = count1;
+ if (count > 0) {
+ stableRowCount = count;
return true;
}
Review Comment:
This no longer waits for the row count to stabilize—only for it to become
non-zero. That can reintroduce timing races (e.g., rows still
rendering/sorting/paginating) and make downstream actions flaky. Consider
reinstating a stabilization check (two consecutive counts separated by a short
delay) or waiting on a UI “loading/ready” signal specific to the table instead
of returning as soon as any row exists.
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -153,50 +137,45 @@ export class ConnectionsPage extends BasePage {
// Delete a connection by connection ID
public async deleteConnection(connectionId: string): Promise<void> {
- // await this.navigate();
const row = await this.findConnectionRow(connectionId);
if (!row) {
throw new Error(`Connection ${connectionId} not found`);
}
- // Find delete button in the row
- await this.page.evaluate(() => {
- const backdrops =
document.querySelectorAll<HTMLElement>('[data-scope="dialog"][data-part="backdrop"]');
-
- backdrops.forEach((backdrop) => {
- const { state } = backdrop.dataset;
-
- if (state === "closed") {
- backdrop.remove();
- }
- });
- });
const deleteButton = row.getByRole("button", { name: "Delete Connection"
});
await expect(deleteButton).toBeVisible({ timeout: 10_000 });
await expect(deleteButton).toBeEnabled({ timeout: 5000 });
await deleteButton.click();
- await expect(this.confirmDeleteButton).toBeVisible({ timeout: 10_000 });
- await expect(this.confirmDeleteButton).toBeEnabled({ timeout: 5000 });
- await this.confirmDeleteButton.click();
+ // Wait for the dialog to finish its open animation (data-state="open" is
set by
+ // Ark UI once the transition completes). Without this, the backdrop can
cover the
+ // confirm button during the animation and cause the click to time out.
+ const deleteDialog =
this.page.locator('[role="dialog"][data-state="open"]');
- await expect(this.emptyState).toBeVisible({ timeout: 5000 });
+ await deleteDialog.waitFor({ state: "visible", timeout: 10_000 });
+ const confirmButton = deleteDialog.getByRole("button", { name: "Yes,
Delete" });
+
+ await expect(confirmButton).toBeVisible({ timeout: 5000 });
+ // force: true bypasses Playwright's hit-testing check — the button is
correct but
+ // the dialog backdrop can still briefly overlap it right after the
animation ends.
+ await confirmButton.click({ force: true });
+
+ await expect(this.getConnectionRow(connectionId)).not.toBeVisible({
timeout: 15_000 });
}
// Edit a connection by connection ID
public async editConnection(connectionId: string, updates:
Partial<ConnectionDetails>): Promise<void> {
- const row = await this.findConnectionRow(connectionId);
-
- if (!row) {
- throw new Error(`Connection ${connectionId} not found`);
- }
-
await this.clickEditButton(connectionId);
- // Wait for form to load
- await expect(this.connectionIdInput).toBeVisible({ timeout: 10_000 });
+ // Wait for form to be fully populated with existing connection data
before interacting
+ await expect(this.connectionIdInput).toHaveValue(connectionId, { timeout:
10_000 });
+
+ // Wait for the form to stabilize — React re-renders the conn_type
combobox as
+ // existing connection data loads from the API, which causes "detached
from DOM"
+ // errors if we interact with it too early.
+ await this.page.waitForLoadState("networkidle", { timeout: 10_000 });
Review Comment:
`waitForLoadState("networkidle")` is often unreliable in SPA-style apps and
can hang or be flaky due to background polling/telemetry requests. Prefer
waiting for a concrete UI condition that indicates the form is ready (e.g., the
conn_type combobox/input has the expected value, a loading indicator
disappears, or waiting for the specific API response that populates the form).
```suggestion
// Wait for form to be fully populated with existing connection data
before interacting.
// Use concrete UI readiness checks instead of `networkidle`, which is
flaky in SPA-style
// apps that may continue background requests while the form is already
ready.
await expect(this.connectionIdInput).toBeVisible({ timeout: 10_000 });
await expect(this.connectionIdInput).toBeEnabled({ timeout: 10_000 });
await expect(this.connectionIdInput).toHaveValue(connectionId, {
timeout: 10_000 });
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -153,50 +137,45 @@ export class ConnectionsPage extends BasePage {
// Delete a connection by connection ID
public async deleteConnection(connectionId: string): Promise<void> {
- // await this.navigate();
const row = await this.findConnectionRow(connectionId);
if (!row) {
throw new Error(`Connection ${connectionId} not found`);
}
- // Find delete button in the row
- await this.page.evaluate(() => {
- const backdrops =
document.querySelectorAll<HTMLElement>('[data-scope="dialog"][data-part="backdrop"]');
-
- backdrops.forEach((backdrop) => {
- const { state } = backdrop.dataset;
-
- if (state === "closed") {
- backdrop.remove();
- }
- });
- });
const deleteButton = row.getByRole("button", { name: "Delete Connection"
});
await expect(deleteButton).toBeVisible({ timeout: 10_000 });
await expect(deleteButton).toBeEnabled({ timeout: 5000 });
await deleteButton.click();
- await expect(this.confirmDeleteButton).toBeVisible({ timeout: 10_000 });
- await expect(this.confirmDeleteButton).toBeEnabled({ timeout: 5000 });
- await this.confirmDeleteButton.click();
+ // Wait for the dialog to finish its open animation (data-state="open" is
set by
+ // Ark UI once the transition completes). Without this, the backdrop can
cover the
+ // confirm button during the animation and cause the click to time out.
+ const deleteDialog =
this.page.locator('[role="dialog"][data-state="open"]');
- await expect(this.emptyState).toBeVisible({ timeout: 5000 });
+ await deleteDialog.waitFor({ state: "visible", timeout: 10_000 });
+ const confirmButton = deleteDialog.getByRole("button", { name: "Yes,
Delete" });
+
+ await expect(confirmButton).toBeVisible({ timeout: 5000 });
+ // force: true bypasses Playwright's hit-testing check — the button is
correct but
+ // the dialog backdrop can still briefly overlap it right after the
animation ends.
+ await confirmButton.click({ force: true });
Review Comment:
Clicking with `force: true` bypasses Playwright’s actionability checks and
can mask real UX regressions (e.g., the button is genuinely not clickable). If
possible, prefer waiting until the button is actually actionable (visible +
enabled + not covered) and then clicking without `force`. If overlap is
transient, a more targeted wait (e.g., for the backdrop/transition state) is
usually safer than forcing the click.
```suggestion
await expect(confirmButton).toBeEnabled({ timeout: 5000 });
// Wait until Playwright's actionability checks pass, including
hit-testing, before
// performing the real click. This avoids masking genuine UI regressions
with force.
await confirmButton.click({ trial: true });
await confirmButton.click();
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -84,22 +84,22 @@ export class ConnectionsPage extends BasePage {
this.loginInput = page.locator('input[name="login"]').first();
this.passwordInput = page.locator('input[name="password"],
input[type="password"]').first();
this.schemaInput = page.locator('input[name="schema"]').first();
- // Try multiple possible selectors
this.descriptionInput = page.locator('[name="description"]').first();
-
// Alerts
this.successAlert = page.locator('[data-scope="toast"][data-part="root"]');
- // Delete confirmation dialog
- this.confirmDeleteButton =
page.locator('button:has-text("Delete")').first();
this.rowsPerPageSelect = page.locator("select");
// Sorting and filtering
- this.tableHeader = page.locator('[role="columnheader"]').first();
- this.connectionIdHeader = page.locator("th:has-text('Connection
ID')").first();
- this.connectionTypeHeader = page.locator('th:has-text("Connection
Type")').first();
- this.hostHeader = page.locator('th:has-text("Host")').first();
- this.searchInput = page.locator('input[placeholder*="Search"],
input[placeholder*="search"]').first();
+ this.tableHeader = page.getByRole("columnheader").first();
+
+ this.connectionIdHeader = page.getByText("Connection ID").first();
+ this.connectionTypeHeader = page.getByText("Connection Type").first();
+ this.hostHeader = page.getByText("Host").first();
Review Comment:
Using `getByText(...).first()` for column headers can match non-header text
(e.g., labels/help text in dialogs) and then “first()” may hide selector
ambiguity. Since these are table headers, prefer role-based selectors scoped to
the header context (e.g., `getByRole("columnheader", { name: "Connection ID"
})`), which is typically more resilient and aligns with Playwright best
practices.
```suggestion
this.tableHeader =
this.connectionsTable.getByRole("columnheader").first();
this.connectionIdHeader =
this.connectionsTable.getByRole("columnheader", { name: "Connection ID" });
this.connectionTypeHeader =
this.connectionsTable.getByRole("columnheader", { name: "Connection Type" });
this.hostHeader = this.connectionsTable.getByRole("columnheader", {
name: "Host" });
```
##########
airflow-core/src/airflow/ui/tests/e2e/specs/connections.spec.ts:
##########
@@ -281,62 +271,35 @@ test.describe("Connections Page - Search and Filter", ()
=> {
test("should filter connections by search term", async () => {
await connectionsPage.navigate();
- const initialCount = await connectionsPage.getConnectionCount();
-
- expect(initialCount).toBeGreaterThan(0);
-
- const searchTerm = "production";
+ const targetConnection = searchTestConnections[0]!;
+ const searchTerm = targetConnection.connection_id;
+ // Check that we have at least one row before searching (web-first
assertion)
await connectionsPage.searchConnections(searchTerm);
- await expect
- .poll(
- async () => {
- const ids = await connectionsPage.getConnectionIds();
-
- // Verify we have results AND they match the search term
- return ids.length > 0 && ids.every((id) =>
id.toLowerCase().includes(searchTerm.toLowerCase()));
- },
- { intervals: [500], timeout: 10_000 },
- )
- .toBe(true);
-
- const filteredIds = await connectionsPage.getConnectionIds();
-
- expect(filteredIds.length).toBeGreaterThan(0);
- for (const id of filteredIds) {
- expect(id.toLowerCase()).toContain(searchTerm.toLowerCase());
- }
+ // Verify filtered results contain the search term
+ await expect(connectionsPage.connectionRows).toHaveCount(1);
+ await expect(connectionsPage.getConnectionRow(searchTerm)).toBeVisible();
Review Comment:
`toHaveCount(1)` bakes in an assumption that searching by `connection_id`
yields exactly one row. If the UI search matches other columns (or other IDs
contain the searched substring), this becomes flaky while the feature still
works. Prefer asserting that a matching row is visible and (optionally) that
all visible rows contain the search term (e.g., via
`expect(connectionsPage.connectionRows).toContainText(...)` or a small
`expect.poll` over row texts) instead of enforcing a fixed count of 1.
```suggestion
// Verify filtered results contain the search term without assuming a
single exact match
await expect(connectionsPage.getConnectionRow(searchTerm)).toBeVisible();
await expect
.poll(async () => {
const rowTexts = await
connectionsPage.connectionRows.allTextContents();
return rowTexts.length > 0 && rowTexts.every((text) =>
text.toLowerCase().includes(searchTerm.toLowerCase()));
})
.toBe(true);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
