Copilot commented on code in PR #63969:
URL: https://github.com/apache/airflow/pull/63969#discussion_r3066473896
##########
airflow-core/src/airflow/ui/tests/e2e/pages/XComsPage.ts:
##########
@@ -169,12 +163,6 @@ export class XComsPage extends BasePage {
await expect(valueCell).toBeVisible();
- await expect(async () => {
- const textContent = await valueCell.textContent();
- const hasTextContent = (textContent?.trim().length ?? 0) > 0;
- const hasWidgetContent = (await valueCell.locator("button, pre,
code").count()) > 0;
-
- expect(hasTextContent || hasWidgetContent).toBeTruthy();
- }).toPass({ timeout: 10_000 });
+ await expect(valueCell).not.toBeEmpty({ timeout: 10_000 });
Review Comment:
This changes the asserted condition: the removed logic passed when the cell
had either non-empty text OR a widget element (`button, pre, code`).
`not.toBeEmpty()` only verifies non-empty text/value, so cases where the UI
renders a non-text widget (e.g., icon-only button) can now fail even though the
cell is meaningfully populated. Consider keeping an assertion that matches the
intended acceptance criteria (text present OR widget present), using locator
assertions/polling rather than `textContent()`.
```suggestion
await expect(async () => {
const widgetCount = await valueCell.locator("button, pre,
code").count();
if (widgetCount === 0) {
await expect(valueCell).toContainText(/\S/);
} else {
expect(widgetCount).toBeGreaterThan(0);
}
}).toPass({ timeout: 10_000 });
```
##########
airflow-core/src/airflow/ui/tests/e2e/specs/xcoms.spec.ts:
##########
@@ -43,20 +43,9 @@ test.describe("XComs Page", () => {
}
await setupXComsPage.navigate();
- await page.waitForFunction(
- (minCount) => {
- const table = document.querySelector('[data-testid="table-list"]');
-
- if (!table) {
- return false;
- }
- const rows = table.querySelectorAll("tbody tr");
-
- return rows.length >= minCount;
- },
- triggerCount,
- { timeout: 120_000 },
- );
+ await expect(page.locator('[data-testid="table-list"] tbody
tr').nth(triggerCount - 1)).toBeVisible({
+ timeout: 120_000,
+ });
Review Comment:
`nth(triggerCount - 1)` changes behavior when `triggerCount` can be `0`: the
previous implementation (`rows.length >= minCount`) would succeed immediately
for `0`, but `nth(-1)` is invalid (and will fail). If `triggerCount` is allowed
to be `0`, handle that case explicitly (e.g., assert the row count is 0 / skip
the `nth` assertion) or use a count-based wait that supports `>= triggerCount`.
##########
airflow-core/src/airflow/ui/tests/e2e/pages/XComsPage.ts:
##########
@@ -57,14 +57,13 @@ export class XComsPage extends BasePage {
await filterInput.waitFor({ state: "visible", timeout: 5000 });
await filterInput.fill(value);
await filterInput.press("Enter");
- await this.page.waitForLoadState("networkidle");
+ await expect(this.xcomsTable.locator("tbody tr").first()).toBeVisible({
timeout: 10_000 });
Review Comment:
Replacing `waitForLoadState(\"networkidle\")` with `first().toBeVisible()`
introduces an assumption that filtering always yields at least one row.
Previously this method would return even if the filter produced zero results;
now it will time out in that case. To keep this a pure “wait for filter
completion” step, wait for a stable UI state that works for both non-empty and
empty results (e.g., empty-state indicator becomes visible, or a
loading/skeleton element disappears, or a deterministic table-refresh marker
changes).
```suggestion
await this.page.waitForLoadState("networkidle");
```
--
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]
