vatsrahul1001 commented on PR #60122:
URL: https://github.com/apache/airflow/pull/60122#issuecomment-3714855200
@Prajwal7842 Thanks for the PR! A few things to address:
1. Test naming - Remove "should" prefix (use "verify ..." instead of "should
verify ...")
2. Remove hardcoded waits - `waitForTimeout(5000)` is slow and fragile. Use
proper Playwright assertions:
await expect(async () => {
const indicator = await eventsPage.getColumnSortIndicator(0);
expect(indicator).not.toBe("none");
}).toPass({ timeout: 10_000 });
3. Move DAG trigger to `beforeAll` - Current "Events with Generated Data"
tests depend on first test running. Move `triggerDag` to `beforeAll` for proper
isolation
4. Column selectors are fragile - Using `.nth(1)`, `.nth(2)` breaks if
column order changes. Use text/role-based selectors:
this.eventTypeColumn = page.getByRole("columnheader", { name: /event/i });
5. Sorting test weak - Only checks indicator, not actual sort order.
Capture data before/after and verify it changed
6. Filter test incomplete - Only opens menu, doesn't apply filter. Add test
that applies filter and verifies results
7. Fix pagination URL - Missing `/` prefix and `limit=1` is too small:
return "/events?limit=5&offset=0";
8. Remove `waitForTimeout` method
Let me know if you have questions!
--
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]
