Copilot commented on code in PR #64191:
URL: https://github.com/apache/airflow/pull/64191#discussion_r3025335266
##########
airflow-core/src/airflow/ui/tests/e2e/pages/RequiredActionsPage.ts:
##########
@@ -76,10 +76,22 @@ export class RequiredActionsPage extends BasePage {
}
private async clickOnTaskInGrid(dagRunId: string, taskId: string):
Promise<void> {
+ const gridContainer = this.page.locator('[data-testid="dag-graph"],
[data-testid^="grid-"]').first();
+
+ await expect(gridContainer).toBeVisible({ timeout: 30_000 });
+
Review Comment:
`gridContainer` uses `[data-testid="dag-graph"], [data-testid^="grid-"]`
with `.first()`. `[data-testid^="grid-"]` is broad (it can match individual
task nodes like `grid-${dagRunId}-${taskId}`), so `.first()` can become
non-deterministic and make this readiness check ineffective. Prefer targeting a
stable container test id (e.g. the graph container) or otherwise scoping the
selector so it cannot match task elements.
```suggestion
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/RequiredActionsPage.ts:
##########
@@ -76,10 +76,22 @@ export class RequiredActionsPage extends BasePage {
}
private async clickOnTaskInGrid(dagRunId: string, taskId: string):
Promise<void> {
+ const gridContainer = this.page.locator('[data-testid="dag-graph"],
[data-testid^="grid-"]').first();
+
+ await expect(gridContainer).toBeVisible({ timeout: 30_000 });
+
const taskLocator = this.page.getByTestId(`grid-${dagRunId}-${taskId}`);
- await expect(taskLocator).toBeVisible({ timeout: 5000 });
- await taskLocator.click();
+ await expect(taskLocator).toBeVisible({ timeout: 30_000 });
+ await taskLocator.click().catch(async () => {
+ const fallbackLocator = this.page
+ .locator(`[data-testid*="${taskId}"]`)
+ .filter({ hasText: taskId })
+ .first();
+
+ await expect(fallbackLocator).toBeVisible({ timeout: 30_000 });
+ await fallbackLocator.click();
+ });
Review Comment:
Using `await taskLocator.click().catch(async () => { ... })` can swallow
relevant Playwright click failures (e.g. element detached, intercepted clicks)
and makes retries harder to reason about. For flaky clicks, prefer Playwright’s
built-in retries via `expect(taskLocator).toBeVisible/toBeEnabled` plus
`taskLocator.click({ trial: true })`/`toPass`, or implement a controlled retry
that re-resolves locators and fails with the original error if the fallback
also fails.
```suggestion
let originalClickError: unknown;
try {
await taskLocator.click();
} catch (error) {
originalClickError = error;
const fallbackLocator = this.page
.locator(`[data-testid*="${taskId}"]`)
.filter({ hasText: taskId })
.first();
await expect(fallbackLocator).toBeVisible({ timeout: 30_000 });
try {
await fallbackLocator.click();
} catch {
throw originalClickError;
}
}
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/DagCalendarTab.ts:
##########
@@ -109,22 +109,18 @@ export class DagCalendarTab extends BasePage {
}
private async waitForCalendarReady(): Promise<void> {
- await this.page.getByTestId("dag-calendar-root").waitFor({ state:
"visible", timeout: 120_000 });
+ await expect(this.page.getByTestId("dag-calendar-root")).toBeVisible({
timeout: 120_000 });
- await this.page.getByTestId("calendar-current-period").waitFor({ state:
"visible", timeout: 120_000 });
+ await
expect(this.page.getByTestId("calendar-current-period")).toBeVisible({ timeout:
120_000 });
const overlay = this.page.getByTestId("calendar-loading-overlay");
if (await overlay.isVisible().catch(() => false)) {
- await overlay.waitFor({ state: "hidden", timeout: 120_000 });
+ await expect(overlay).toBeHidden({ timeout: 120_000 });
}
- await this.page.getByTestId("calendar-grid").waitFor({ state: "visible",
timeout: 120_000 });
+ await expect(this.page.getByTestId("calendar-grid")).toBeVisible({
timeout: 120_000 });
- await this.page.waitForFunction(() => {
- const cells = document.querySelectorAll('[data-testid="calendar-cell"]');
-
- return cells.length > 0;
- });
+ await expect(this.calendarCells.first()).toBeVisible({ timeout: 120_000 });
Review Comment:
`await expect(this.calendarCells.first()).toBeVisible(...)` verifies at
least one cell is visible, but it doesn’t ensure the grid is fully
populated/stable (e.g. only a skeleton cell rendered). If the intent is
“calendar fully loaded”, consider asserting on an expected minimum count of
cells (via `toHaveCount` with a range/known value) or waiting for a specific UI
marker that indicates render completion.
```suggestion
await expect(this.calendarCells).toHaveCount(42, { timeout: 120_000 });
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -215,7 +215,12 @@ export class ConnectionsPage extends BasePage {
await expect(selectCombobox).toBeEnabled({ timeout: 25_000 });
- await selectCombobox.click({ timeout: 3000 });
+ await selectCombobox.click({ timeout: 15_000 });
+
+ // Wait for the listbox to be visible before selecting
+ const listbox = this.page.locator('[role="listbox"]');
Review Comment:
`const listbox = this.page.locator('[role="listbox"]')` is unscoped and may
match multiple listboxes (or a hidden one) elsewhere on the page, causing
`toBeVisible` to wait on the wrong element and making the subsequent option
selection flaky. Consider scoping the listbox to the opened combobox (e.g. via
`aria-controls`/`aria-owns` on the combobox), or use
`this.page.getByRole('listbox').first()` together with checks that it appears
after the click.
```suggestion
const listbox = this.page.getByRole("listbox").first();
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/DagCalendarTab.ts:
##########
@@ -109,22 +109,18 @@ export class DagCalendarTab extends BasePage {
}
private async waitForCalendarReady(): Promise<void> {
- await this.page.getByTestId("dag-calendar-root").waitFor({ state:
"visible", timeout: 120_000 });
+ await expect(this.page.getByTestId("dag-calendar-root")).toBeVisible({
timeout: 120_000 });
- await this.page.getByTestId("calendar-current-period").waitFor({ state:
"visible", timeout: 120_000 });
+ await
expect(this.page.getByTestId("calendar-current-period")).toBeVisible({ timeout:
120_000 });
const overlay = this.page.getByTestId("calendar-loading-overlay");
if (await overlay.isVisible().catch(() => false)) {
- await overlay.waitFor({ state: "hidden", timeout: 120_000 });
+ await expect(overlay).toBeHidden({ timeout: 120_000 });
}
- await this.page.getByTestId("calendar-grid").waitFor({ state: "visible",
timeout: 120_000 });
+ await expect(this.page.getByTestId("calendar-grid")).toBeVisible({
timeout: 120_000 });
- await this.page.waitForFunction(() => {
- const cells = document.querySelectorAll('[data-testid="calendar-cell"]');
-
- return cells.length > 0;
- });
+ await expect(this.calendarCells.first()).toBeVisible({ timeout: 120_000 });
Review Comment:
The overlay wait is conditional on a one-time `overlay.isVisible()` check.
If the overlay appears shortly after this check (race during render), the
method will skip waiting for it to disappear and proceed while the grid is
still loading. Prefer an unconditional web-first wait that tolerates both cases
(e.g. `await expect(overlay).toBeHidden()` without the `if`, or wait for the
grid/cells to be visible+stable in a way that also covers late-appearing
overlays).
##########
airflow-core/src/airflow/ui/tests/e2e/pages/XComsPage.ts:
##########
@@ -61,9 +61,11 @@ export class XComsPage extends BasePage {
}
public async navigate(): Promise<void> {
- await this.navigateTo(XComsPage.xcomsUrl);
- await this.page.waitForURL(/.*xcoms/, { timeout: 15_000 });
- await this.xcomsTable.waitFor({ state: "visible", timeout: 10_000 });
+ await expect(async () => {
+ await this.navigateTo(XComsPage.xcomsUrl);
+ await expect(this.page).toHaveURL(/.*xcoms/, { timeout: 15_000 });
+ }).toPass({ intervals: [2000], timeout: 60_000 });
Review Comment:
Wrapping navigation in `expect(async () => { ... }).toPass(...)` retries a
full `page.goto()` on any failure. If the page loads but the second attempt
starts, this can add unnecessary navigation churn and hide the underlying
failure. Where possible, prefer a single `navigateTo` and then use Expect’s
web-first assertions to wait for stable UI state (URL + table visibility). If
you do need retries, consider adding a message or narrowing the retried
assertions to avoid repeated navigation.
```suggestion
await this.navigateTo(XComsPage.xcomsUrl);
await expect(this.page).toHaveURL(/.*xcoms/, { timeout: 60_000 });
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/DagsPage.ts:
##########
@@ -345,8 +345,10 @@ export class DagsPage extends BasePage {
public async triggerDag(dagName: string): Promise<string | null> {
await expect(async () => {
await this.navigateToDagDetail(dagName);
- await expect(this.triggerButton).toBeVisible({ timeout: 5000 });
- }).toPass({ intervals: [2000], timeout: 60_000 });
+ await this.page.waitForLoadState("domcontentloaded");
+ await expect(this.triggerButton).toBeAttached({ timeout: 15_000 });
+ await expect(this.triggerButton).toBeEnabled({ timeout: 15_000 });
+ }).toPass({ intervals: [2000], timeout: 90_000 });
await this.triggerButton.click();
Review Comment:
`triggerDag` now waits for the trigger button to be attached+enabled inside
the 90s `toPass`, but the actual `click()` is outside that block and will use
Playwright’s default action timeout. If the button is attached/enabled but
still not visible/interactable (e.g. covered by a loading overlay), the click
can time out earlier and become flaky. Consider asserting `toBeVisible()` (or
performing the click) inside the same `toPass`, or pass an explicit `timeout`
to `this.triggerButton.click()` consistent with the outer retry timeout.
```suggestion
await expect(this.triggerButton).toBeVisible({ timeout: 15_000 });
}).toPass({ intervals: [2000], timeout: 90_000 });
await this.triggerButton.click({ timeout: 90_000 });
```
##########
airflow-core/src/airflow/ui/tests/e2e/pages/DagCalendarTab.ts:
##########
@@ -85,7 +85,7 @@ export class DagCalendarTab extends BasePage {
public async navigateToCalendar(dagId: string) {
await expect(async () => {
await this.page.goto(`/dags/${dagId}/calendar`);
- await this.page.getByTestId("dag-calendar-root").waitFor({ state:
"visible", timeout: 5000 });
+ await expect(this.page.getByTestId("dag-calendar-root")).toBeVisible({
timeout: 5000 });
}).toPass({ intervals: [2000], timeout: 60_000 });
await this.waitForCalendarReady();
Review Comment:
PR description says the change “specifically” focuses on `DagCalendarTab`,
but this PR also changes other page objects (`XComsPage`, `DagsPage`,
`RequiredActionsPage`, `ConnectionsPage`). Please either update the PR
description/scope or split the unrelated refactors so reviewers can validate
intent and risk more easily.
##########
airflow-core/src/airflow/ui/tests/e2e/pages/ConnectionsPage.ts:
##########
@@ -215,7 +215,12 @@ export class ConnectionsPage extends BasePage {
await expect(selectCombobox).toBeEnabled({ timeout: 25_000 });
- await selectCombobox.click({ timeout: 3000 });
+ await selectCombobox.click({ timeout: 15_000 });
+
+ // Wait for the listbox to be visible before selecting
+ const listbox = this.page.locator('[role="listbox"]');
+
+ await expect(listbox).toBeVisible({ timeout: 15_000 });
// Wait for options to appear and click the matching option
const option = this.page.getByRole("option", { name: new
RegExp(details.conn_type, "i") }).first();
Review Comment:
The listbox wait uses an unscoped selector (`[role="listbox"]`), which can
match multiple listboxes or a hidden one on the page. This can make
`toBeVisible` pass/fail against the wrong element and lead to flaky option
selection. Prefer scoping the listbox to the opened combobox (e.g. via the
combobox `aria-controls` target) or at least selecting the specific listbox
that appears after the click.
```suggestion
// Resolve the listbox associated with this combobox and wait for it
to be visible
const ariaControls = await
selectCombobox.getAttribute("aria-controls");
const listbox = ariaControls
? this.page.locator(`#${ariaControls}`)
: this.page.locator('[role="listbox"]').first();
await expect(listbox).toBeVisible({ timeout: 15_000 });
// Wait for options to appear and click the matching option within
this listbox
const option = listbox.getByRole("option", { name: new
RegExp(details.conn_type, "i") }).first();
```
--
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]
