-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add integration tests for /events Audit log page #60122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
71c92e3 to
8b842f2
Compare
|
@vatsrahul1001 can you take a look. Builds are passing now. Thanks |
|
Thanks, I will review it soon! |
|
@Prajwal7842 Thanks for the PR! A few things to address:
Let me know if you have questions! |
| await eventsPage.navigate(); | ||
| await eventsPage.waitForEventsTable(); | ||
|
|
||
| await eventsPage.waitForTimeout(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds 10 seconds of unnecessary wait time
await expect(async () => {
const sortIndicator = await eventsPage.getColumnSortIndicator(0);
expect(sortIndicator).not.toBe("none");
}).toPass({ timeout: 10_000 });
| test.setTimeout(3 * 60 * 1000); // 3 minutes timeout | ||
|
|
||
| // First, trigger a DAG to generate audit log entries | ||
| await dagsPage.triggerDag(testDagId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move trigger dag to beforeAll
test.describe("Events with Generated Data", () => {
const testDagId = testConfig.testDag.id;
test.beforeAll(async ({ browser }) => {
test.setTimeout(3 * 60 * 1000);
const context = await browser.newContext({ storageState: AUTH_FILE });
const page = await context.newPage();
const dagsPage = new DagsPage(page);
await dagsPage.triggerDag(testDagId);
await context.close();
});
test.beforeEach(({ page }) => {
eventsPage = new EventsPage(page);
});
| this.eventsTable = page.getByRole("table"); | ||
| this.filterBar = page.locator('div:has(button:has-text("Filter"))').first(); | ||
| // Use table header selectors - will check for presence of key columns | ||
| this.timestampColumn = page.locator("thead th").first(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If column order changes, all tests break.
Maybe we can use something like below
this.timestampColumn = page.getByRole("columnheader", { name: /when/i });
this.eventTypeColumn = page.getByRole("columnheader", { name: /event/i });
this.userColumn = page.getByRole("columnheader", { name: /user/i });
| await expect(eventsPage.eventsTable).toBeVisible({ timeout: 10_000 }); | ||
| }); | ||
|
|
||
| test("should verify column sorting works", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't verify the data is actually sorted.
Capture data before/after and verify order
| eventsPage = new EventsPage(page); | ||
| }); | ||
|
|
||
| test("should verify search input is visible", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test only checks that the filter UI exists:
- Opens filter menu
- Verifies menu items visible
- Closes menu
This is NOT a filter test - it's just checking the filter UI is present.
Thanks @vatsrahul1001 for the review. I have addressed the comments. |
28790a3 to
b76c454
Compare
|
@Prajwal7842 #59734 is already merged now. Can you rebase. I think you can reuse alot of code |
|
@Prajwal7842 How are we progressing on this? |
|
b76c454 to
eab74e8
Compare
|
@vatsrahul1001 updated the tests as per suggestions. PTAL. Thanks |
|
Thanks @Prajwal7842 I will review soon |
| const menuItem = filterMenu.locator(`[role="menuitem"]:has-text("${filterName}")`); | ||
|
|
||
| await menuItem.click(); | ||
| await this.page.waitForTimeout(300); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use explicit wait
| @@ -21,8 +21,10 @@ import { BasePage } from "tests/e2e/pages/BasePage"; | |||
|
|
|||
| export class EventsPage extends BasePage { | |||
| public readonly eventColumn: Locator; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are defined but not directly used:
eventColumn, extraColumn, ownerColumn, whenColumn
clickColumnHeader() (only clickColumnToSort() is used)
Consider removing if not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, these columns are being used in dag-audit-log.spec.ts and its usage is added into the previous diff, hence will be keeping this.
| const eventText = await eventCell.textContent(); | ||
| const userText = await userCell.textContent(); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use this
expect(whenText?.trim()).toBeTruthy();
8d66c9a to
a01107f
Compare
|
@Prajwal7842 I see tests are failing. can you check? |
a01107f to
343ad0a
Compare
This PR adds playwright integration tests for /events Audit log page for Airflow UI.
Covers following things from listed down in the issues:
Audit Logs Display
Search/Filter Logs
closes: #59931
releated: #59028
The integration tests were locally run 5+ times consecutively and all runs passed successfully to avoid any flakiness

Sample run: