Prajwal7842 commented on PR #60122:
URL: https://github.com/apache/airflow/pull/60122#issuecomment-3723983381
> @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
> 5. Filter test incomplete - Only opens menu, doesn't apply filter. Add
test that applies filter and verifies results
> 6. 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!
Thanks @vatsrahul1001 for the review. I have addressed the comments.
For point (5), regarding integration tests for filter, I am working on it in
the follow up diff. Thanks.
--
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]
