feat: Add E2E tests for Task Instances page #60514
Conversation
| await taskInstancesPage.verifyPagination(5); | ||
| }); | ||
|
|
||
| test("verify different task states display visually distinct", async () => { |
There was a problem hiding this comment.
We do not need this. Lets remove color checking
| /** | ||
| * Apply a filter by selecting a filter type and value | ||
| */ | ||
| public async applyFilter(filterName: string, filterValue: string): Promise<void> { |
There was a problem hiding this comment.
DAG Runs PR uses URL parameters for filtering which is simpler and more reliable.
| /** | ||
| * Verify pagination works correctly with offset and limit parameters | ||
| */ | ||
| public async verifyPagination(limit: number): Promise<void> { |
There was a problem hiding this comment.
current implementation parses pagination text with regex which is complex and fragile. DAG Runs PR uses a simpler approach
| ); | ||
| const countAfter = await rowsAfterFilter.count(); | ||
|
|
||
| if (countAfter === 0) { |
There was a problem hiding this comment.
should we use
await expect(dataLinks.first().or(noDataMessage.first())).toBeVisible({ timeout: 30_000 });
- Use URL parameters (?task_state=) instead of UI interactions for filtering - Simplify pagination verification using semantic button selectors - Replace regex-based text parsing with Playwright's getByRole() - Use or() pattern for cleaner conditional logic
airflow-core/src/airflow/ui/tests/e2e/pages/TaskInstancesPage.ts
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/tests/e2e/pages/TaskInstancesPage.ts
Outdated
Show resolved
Hide resolved
|
Thanks, @lin121291 for implementing review comments. I suggest resolving comments after you are done. I will review this soon |
| .isVisible() | ||
| .catch(() => false); | ||
|
|
||
| if (hasPage2) { |
There was a problem hiding this comment.
Silently passes if there is not enough data. I think we should not have this condition. As we are already creating data in beforeAll
|
|
||
| await expect(paginationNav.first()).toBeVisible({ timeout: 10_000 }); | ||
|
|
||
| const page1Button = this.page.getByRole("button", { name: /page 1|^1$/ }); |
There was a problem hiding this comment.
We can better use
this.paginationNextButton = page.locator('[data-testid="next"]');
this.paginationPrevButton = page.locator('[data-testid="prev"]');
| const dataLinksPage2 = this.taskInstancesTable.locator("a[href*='/dags/']"); | ||
| const noDataMessage = this.page.locator("text=/no.*data|no.*task instances|no.*results/i"); | ||
|
|
||
| await expect(dataLinksPage2.first().or(noDataMessage.first())).toBeVisible({ timeout: 30_000 }); |
There was a problem hiding this comment.
We are not verifying anything in second page. Atlest we should check data is changed
|
Pagination needs to follow the established pattern from PR #59400: The current implementation differs from our merged pagination tests. Please reference:
// Reference pattern (from DagsPage.ts)
|
2 participants
What change does this PR introduce?
Adds end-to-end (E2E) tests for the Task Instances page (/task_instances) to improve test coverage of the UI.
What does this PR do?
New Files:
airflow-core/src/airflow/ui/tests/e2e/pages/TaskInstancesPage.ts- Page Object Model for Task Instances pageairflow-core/src/airflow/ui/tests/e2e/specs/task-instances.spec.ts- Test specificationsRelated Issues
Closes #59357