-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add E2E tests for Plugins page #61057
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
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
| test.describe("Plugins Pagination", () => { | ||
| let pluginsPage: PluginsPage; | ||
|
|
||
| test.beforeEach(async ({ page }) => { |
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.
I think you have to remove async to fix CI
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.
Applied! Thanks
- Create PluginsPage page object following POM pattern - Add test specs for plugins list display and pagination - Tests work across Chromium, Firefox, and WebKit browsers - All 24 tests passing Fixes apache#60571
c9e7eaa to
42589cf
Compare
| const pagination = pluginsPage.page.locator('[data-scope="pagination"]'); | ||
| const paginationExists = await pagination.isVisible().catch(() => false); | ||
|
|
||
| if (paginationExists) { |
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.
Either ensure enough data exists in the test environment, or fail explicitly if pagination isn't available
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.
await expect(nextButton).toBeVisible(); await expect(nextButton).toBeEnabled();
|
|
||
| if (paginationExists) { | ||
| // Check if page 2 button exists and is enabled | ||
| const page2Button = pagination.getByRole("button", { name: /page 2/i }); |
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 we have already defined page.locator('[data-testid="next"]'); we should use that right?
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.
Consider consolidating to one pagination test using next/prev buttons to align with established patterns.
|
Thanks for this PR @Eason09053360 I have added some review comment. |
|
Thanks for reviewing !You're absolutely right. I've updated the tests to follow the established patterns from Changes :
The test now explicitly fails if pagination isn't available, rather than silently passing. |
@Eason09053360 can you look at failing tests |
… currently fails in CI due to test environment having only 7 plugins, which is insufficient to trigger pagination UI controls. Seeking guidance on whether to keep failing test or remove it.
|
I've implemented (fail explicitly) as shown in the current code. The test will fail with a clear error message when pagination buttons are not visible. I think there are some problems here. 1.Tests are explicit about the limitation (environment has only 7 built-in plugins) Should I keep the failing pagination test to highlight the environment limitation? |
|
I see @vishakha1411 has fixed pagination for plugins in PR |
|
Thank you for pointing me to PR #61059. I've reviewed the fix and understand that Plugins.tsx now correctly implements pagination support However, the pagination E2E test still fails in the current test environment. I tested with limit=5&offset=0 to trigger pagination: Expected: 2 pages (5 plugins on page 1, 2 on page 2) → pagination controls should appear When navigating to /plugins?limit=5&offset=0, the URL parameters seem to get cleared during React initialization, resulting in all 7 plugins being displayed on a single page without pagination controls. |
@Eason09053360 I suggest you to check how we are handling this in eventpage test |

Add E2E tests for Plugins page (/plugins) to verify plugins list display and pagination functionality.
This PR implements:
Closes: #60571
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot following the guidelines