vatsrahul1001 commented on PR #59939: URL: https://github.com/apache/airflow/pull/59939#issuecomment-3765625370
> Hi @vatsrahul1001, thanks for the detailed review! I've addressed all your feedback: > > ### Changes Made: > 1. **Coordinated with PR [Test: Add E2E tests for Assets Page #59990](https://github.com/apache/airflow/pull/59990)** - Renamed `AssetsPage.ts` → `AssetDetailPage.ts` to clearly distinguish from the merged `AssetListPage.ts`. Also renamed the test describe to `"Asset Details Page"`. > 2. **Added `beforeAll` for data setup** - Added setup that triggers `asset_produces_1` DAG and polls for completion before tests run, ensuring asset data exists in CI. > 3. **Fixed fragile selectors**: > > * Replaced `label.locator("..")` with `label.locator("xpath=./parent::*")` + `.first()` > * Replaced `.chakra-popover__body` with `getByRole("dialog").getByRole("link")` > 4. **Test naming** - Changed to `"verify asset details and dependencies"` (removed "should") > 5. **Removed unused code** - Removed the `assetsTable` locator > 6. **Refactored duplicate code** - Created `verifyStatSection()` helper method used by both `verifyProducingTasks()` and `verifyScheduledDags()` > > Tests pass locally on Chromium and WebKit. Ready for re-review! Please have a look and if need any furthur changes let me know! @junaiddshaukat Thanks I will review soon -- 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]
