Skip to content
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

HDDS-9791. Add tests for Datanodes page #7626

Merged
merged 6 commits into from
Jan 2, 2025

Conversation

devabhishekpal
Copy link
Contributor

What changes were proposed in this pull request?

HDDS-9791. Add tests for Datanodes page

Please describe your PR in detail:

  • This PR adds unit tests for the new Datanodes page, and the Datanodes Table component
  • We are also adding an extra library @testing-library/user-event to help with triggering various UI events like clicks, change, type etc.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9791

How was this patch tested?

Patch was tested manually by running all the tests including previous Overview page tests
Screenshot 2024-12-30 at 05 22 46

@devabhishekpal
Copy link
Contributor Author

Hi @ivandika3 @devmadhuu @ArafatKhan2198 could you help take a look at this PR?

@ArafatKhan2198
Copy link
Contributor

Thanks for the effort on this @devabhishekpal

From the looks of it The existing patch successfully tests table rendering, data loading on page mount, row selection, search filtering, API error handling, static and empty API responses, edge cases for search results, data display in rows, and integration with mock servers.

The tests are missing coverage for features like:

  1. Pagination functionality: Ensuring proper navigation between pages of datanodes.
  2. Sorting behavior: Verifying the sorting functionality for each column.
  3. Column selection dropdown: Testing the "Columns" dropdown for adding/removing table columns.

Let me know if we are planning to integrate them in this scope.

@ArafatKhan2198
Copy link
Contributor

ArafatKhan2198 commented Jan 2, 2025

@devabhishekpal Could you also test out this scenario as well :- Search input edge cases (e.g., special characters, case sensitivity).

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the patch @devabhishekpal
LGTM +1

@devabhishekpal
Copy link
Contributor Author

Thanks for the effort on this @devabhishekpal

From the looks of it The existing patch successfully tests table rendering, data loading on page mount, row selection, search filtering, API error handling, static and empty API responses, edge cases for search results, data display in rows, and integration with mock servers.

The tests are missing coverage for features like:

  1. Pagination functionality: Ensuring proper navigation between pages of datanodes.
  2. Sorting behavior: Verifying the sorting functionality for each column.
  3. Column selection dropdown: Testing the "Columns" dropdown for adding/removing table columns.

Let me know if we are planning to integrate them in this scope.

  • So for pagination we will be getting an error as AntD internally calls scrollToTop which is not present in a virtual DOM environment - while this one won't cause any test to fail, it might cause false positives as the exception is being thrown from an internal library.
  • Sorting behaviour is better to be validated as a functional test via something like Cypress on an actual cluster rather than using dummy data. Similarly other test cases like testing out with large dataset (1000+ rows), auto-refresh functionality validation etc. will be better suited in a functional test environment (acceptance test) via docker.
  • For column selection the test cases need to be added for the respective multiSelect.tsx file which isn't in this scope and will be added later on as a part of component unit tests (search, selection etc.)

@ArafatKhan2198
Copy link
Contributor

Thanks for the patch @devabhishekpal
Merging!

@ArafatKhan2198 ArafatKhan2198 merged commit 9aae7a5 into apache:master Jan 2, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants