improve e2e tests for dag audit log#63463
Conversation
d86eb8a to
c192e95
Compare
17ad933 to
7a46501
Compare
sjyangkevin
left a comment
There was a problem hiding this comment.
Hi @choo121600 , thank you for your time to review. I found that the tests are still not very stable after making the changes in the PR. I still encountered failure from time to time, and sometime not re-produceable , and will take some time to look into it. Not sure if you've experienced with the following situations.
|
@sjyangkevin Could you share the command you used to run the tests? |
Thanks and agree, I use breeze to run only test for event page and for all browser. |
|
@sjyangkevin This PR has a few issues that need to be addressed before it can be reviewed — please see our Pull Request quality criteria. Issues found:
What to do next:
There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
Running tests across multiple browsers simultaneously can cause race conditions. In particular, when several browsers check the state of the same Dag, it may lead to inconsistent results. This is a known issue that we plan to address in the future. If you're interested, exploring possible solutions would be a great contribution :) |
Yes. I think I encountered similar issues when testing a change to audit log. Since we are testing multiple browsers against one Airflow server. Each browser beforeAll runs the DAGs multiple times, and cause the tests run against a changing set of data (when some tests are running, DAG is still running, and inserting records, etc.) I feel there are multiple ways might be feasible, and happy to discuss in further details.
|
715fe60 to
52f4b12
Compare
|
It looks like the updates resulting in the following timeout when the tests are running against all browsers at the same time. I attempted to run each browser one-by-one, and all tests passed. 🤔 |
|
|
d13f98c to
8587f0c
Compare
|
I noticed a small improvement in Currently, the test extracts text using It might be more robust to use locator-based assertions instead, for example: await expect(whenCell).toHaveText(/\S+/);
await expect(eventCell).toHaveText(/\S+/);
await expect(userCell).toHaveText(/\S+/);This keeps the assertions within Playwright’s retry mechanism and could potentially help with some of the intermittent stability issues mentioned above. Happy to help implement this if it sounds reasonable 🙂 |
|
I will push one small change to improve the audit log show valid data, to use more explicit matching pattern instead of verifying only not empty string, during the weekend, bandwidth a bit limited this week. Thanks |
|
I am facing some issues with CI locally, and wasn't able to push the changes, working on it. |
0536a19 to
df8fefc
Compare
setFilterValue() previously used getByPlaceholder(filterLabel) to find the
filter input after selecting a filter from the menu. This never matched
because InputWithAddon renders the label as a visible <Text> sibling of the
<input>, not as a placeholder attribute. The filter configs (e.g. EVENT_TYPE,
DAG_ID) do not define a placeholder field, so filter.config.placeholder is
undefined and React omits the attribute from the DOM entirely.
The fix uses the text-matching approach already established in XComsPage:
page.locator("div").filter({ hasText: `${filterLabel}:` }).locator("input").first()
Note: getByPlaceholder() could be restored once the relevant filter configs
in filterConfigs.tsx expose a placeholder value, or once InputWithAddon gains
an aria-label prop (enabling the preferred getByLabel() approach). Both
require a coordinated source change discussed with UI maintainers.
Additional stability improvements:
- Raise addFilter() menu visibility timeout 5s → 10s (CI headroom)
- Raise setFilterValue() input visibility timeout 5s → 10s (CI headroom)
- Raise dag-audit-log individual test timeout 60s → 120s; waitForTableLoad()
can consume up to 60s internally, leaving no room for navigation + assertions
under the old limit on loaded CI runners
df8fefc to
a32d643
Compare
Thanks for all the feedback and review! I also learned new things from it! |
* improve e2e tests for dag audit log
* refine matching on filter
* improve by never snapshot the DOM to assert against the snapshot
* fix skeleton load
* Fix filter input locator and stabilize dag audit log e2e tests
setFilterValue() previously used getByPlaceholder(filterLabel) to find the
filter input after selecting a filter from the menu. This never matched
because InputWithAddon renders the label as a visible <Text> sibling of the
<input>, not as a placeholder attribute. The filter configs (e.g. EVENT_TYPE,
DAG_ID) do not define a placeholder field, so filter.config.placeholder is
undefined and React omits the attribute from the DOM entirely.
The fix uses the text-matching approach already established in XComsPage:
page.locator("div").filter({ hasText: `${filterLabel}:` }).locator("input").first()
Note: getByPlaceholder() could be restored once the relevant filter configs
in filterConfigs.tsx expose a placeholder value, or once InputWithAddon gains
an aria-label prop (enabling the preferred getByLabel() approach). Both
require a coordinated source change discussed with UI maintainers.
Additional stability improvements:
- Raise addFilter() menu visibility timeout 5s → 10s (CI headroom)
- Raise setFilterValue() input visibility timeout 5s → 10s (CI headroom)
- Raise dag-audit-log individual test timeout 60s → 120s; waitForTableLoad()
can consume up to 60s internally, leaving no room for navigation + assertions
under the old limit on loaded CI runners
* use getByRole instead of hasText
* fix filterInput, locate the container and get text
* improve verify audit log entries by explicit matching patterns
* improve e2e tests for dag audit log
* refine matching on filter
* improve by never snapshot the DOM to assert against the snapshot
* fix skeleton load
* Fix filter input locator and stabilize dag audit log e2e tests
setFilterValue() previously used getByPlaceholder(filterLabel) to find the
filter input after selecting a filter from the menu. This never matched
because InputWithAddon renders the label as a visible <Text> sibling of the
<input>, not as a placeholder attribute. The filter configs (e.g. EVENT_TYPE,
DAG_ID) do not define a placeholder field, so filter.config.placeholder is
undefined and React omits the attribute from the DOM entirely.
The fix uses the text-matching approach already established in XComsPage:
page.locator("div").filter({ hasText: `${filterLabel}:` }).locator("input").first()
Note: getByPlaceholder() could be restored once the relevant filter configs
in filterConfigs.tsx expose a placeholder value, or once InputWithAddon gains
an aria-label prop (enabling the preferred getByLabel() approach). Both
require a coordinated source change discussed with UI maintainers.
Additional stability improvements:
- Raise addFilter() menu visibility timeout 5s → 10s (CI headroom)
- Raise setFilterValue() input visibility timeout 5s → 10s (CI headroom)
- Raise dag-audit-log individual test timeout 60s → 120s; waitForTableLoad()
can consume up to 60s internally, leaving no room for navigation + assertions
under the old limit on loaded CI runners
* use getByRole instead of hasText
* fix filterInput, locate the container and get text
* improve verify audit log entries by explicit matching patterns
* improve e2e tests for dag audit log
* refine matching on filter
* improve by never snapshot the DOM to assert against the snapshot
* fix skeleton load
* Fix filter input locator and stabilize dag audit log e2e tests
setFilterValue() previously used getByPlaceholder(filterLabel) to find the
filter input after selecting a filter from the menu. This never matched
because InputWithAddon renders the label as a visible <Text> sibling of the
<input>, not as a placeholder attribute. The filter configs (e.g. EVENT_TYPE,
DAG_ID) do not define a placeholder field, so filter.config.placeholder is
undefined and React omits the attribute from the DOM entirely.
The fix uses the text-matching approach already established in XComsPage:
page.locator("div").filter({ hasText: `${filterLabel}:` }).locator("input").first()
Note: getByPlaceholder() could be restored once the relevant filter configs
in filterConfigs.tsx expose a placeholder value, or once InputWithAddon gains
an aria-label prop (enabling the preferred getByLabel() approach). Both
require a coordinated source change discussed with UI maintainers.
Additional stability improvements:
- Raise addFilter() menu visibility timeout 5s → 10s (CI headroom)
- Raise setFilterValue() input visibility timeout 5s → 10s (CI headroom)
- Raise dag-audit-log individual test timeout 60s → 120s; waitForTableLoad()
can consume up to 60s internally, leaving no room for navigation + assertions
under the old limit on loaded CI runners
* use getByRole instead of hasText
* fix filterInput, locate the container and get text
* improve verify audit log entries by explicit matching patterns
* improve e2e tests for dag audit log
* refine matching on filter
* improve by never snapshot the DOM to assert against the snapshot
* fix skeleton load
* Fix filter input locator and stabilize dag audit log e2e tests
setFilterValue() previously used getByPlaceholder(filterLabel) to find the
filter input after selecting a filter from the menu. This never matched
because InputWithAddon renders the label as a visible <Text> sibling of the
<input>, not as a placeholder attribute. The filter configs (e.g. EVENT_TYPE,
DAG_ID) do not define a placeholder field, so filter.config.placeholder is
undefined and React omits the attribute from the DOM entirely.
The fix uses the text-matching approach already established in XComsPage:
page.locator("div").filter({ hasText: `${filterLabel}:` }).locator("input").first()
Note: getByPlaceholder() could be restored once the relevant filter configs
in filterConfigs.tsx expose a placeholder value, or once InputWithAddon gains
an aria-label prop (enabling the preferred getByLabel() approach). Both
require a coordinated source change discussed with UI maintainers.
Additional stability improvements:
- Raise addFilter() menu visibility timeout 5s → 10s (CI headroom)
- Raise setFilterValue() input visibility timeout 5s → 10s (CI headroom)
- Raise dag-audit-log individual test timeout 60s → 120s; waitForTableLoad()
can consume up to 60s internally, leaving no room for navigation + assertions
under the old limit on loaded CI runners
* use getByRole instead of hasText
* fix filterInput, locate the container and get text
* improve verify audit log entries by explicit matching patterns
Summary
1.
page.locator('[data-testid="..."]')→page.getByTestId("...")**getByTestIdis a semantic, first-class Playwright API for test-id lookups. It's shorter and communicates intent clearly.2.
:has-text()→getByRole()**:has-text()is a Playwright-specific CSS pseudo-selector extension rather than standard CSS and not resilient to structural changes.getByRolequeries the accessibility tree, matching how real users and assistive technologies perceive the element.3.
:has-text()→getByRole().filter()Capture column header and avoid time-out due to matching issue, narrow by visible text. The
filter()method uses Playwright's internal text matching rather than a CSS:has-text().4.
filterBar,addFilter(),:has-text()button selectors →getByRole()getByRole("button", { name: "Filter" })queries by accessible name, which is exactly how screen readers identify buttons. For the menu items, the accessible name is the item's text, so this works directly without a CSS pseudo-selector.5.
tableRows,locator("tbody tr")→locator("tbody").getByRole("row")getByRole("row")queries by ARIA role rather than HTML tag, making it resilient to markup changes (e.g., if<tr>were ever replaced with arole="row" <div>). Chaining.locator("tbody")first scopes the search to body rows only, excluding the header row.6.
waitForTableLoad(),waitForFunction()→ web-first assertions;beforeAllpolling,waitForFunction()→expect().toPass()Same
waitForFunctionanti-pattern.expect().toPass()keeps all locator logic in Playwright's layer (with proper retries and error messages), instead of dropping into raw DOM manipulation inside a browser-evaluated function.Was generative AI tooling used to co-author this PR?
Generated-by: [Claude Code (claude-sonnet-4-6)] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.