-
Notifications
You must be signed in to change notification settings - Fork 16.5k
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
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 |
|
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 |
|
I've implemented the pagination test following the EventsPage pattern as suggested. However, I discovered a frontend issue during testing: Test Results:
Investigation:
|
Srabasti
left a comment
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.
Congratulation for your first PR @Eason09053360!
For the failing static checks, please use " (double quote) instead of single quote. Multiple lines have this issue in addition to here.
Have you reproduced locally by running prek in your branch? Prek will fix any formatting errors, and then you can push the commit from your branch.
|
@Eason09053360 can you take a look at the failures? |
- 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
… 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.
- Added debug logging to verify URL params and actual plugin count - Confirmed URL has limit=5 but page displays all 7 plugins - Root cause: Frontend not applying URL limit parameter on initial load - Backend API works correctly (verified in test_plugins.py)
- Reorder methods alphabetically in PluginsPage.ts - Apply code style formatting to plugins.spec.ts - Consolidate pagination test to align with DagsPage pattern - Remove debug logging - Fix async/await linting error
ffa2629 to
610bc0b
Compare
|
Thanks for reviewing! I rebased the branch to the latest main and the CI passed. It seems the previous failures were unrelated flaky tests. |


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