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

Jest table filter tests on the overview pages #1212

Merged
merged 6 commits into from Mar 1, 2023
Merged

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Feb 22, 2023

Description

Move tables filtering tests from cypress to jest. This makes the testing more reliable. Besides, now all filters are covered, and adding new scenarios is pretty simple.
The tests are pretty much identical across all the table views: hosts, clusters, sap systems and databases, but I have decided to add all of them. This is mainly because the filter function for each filter box is specific.

The code is based in scenario tables, which later on are used in the test. Even though the code looks similar, some duplication is better here, to avoid having more complex code.

The most generic table filtering tests are done in the specific Table.test.jsx file, as we don't really need specific tables and data for it. (Edit: now done in: #1216)

Finally, it removes the cypress tests (Edit: now done in: #1215)

@arbulu89 arbulu89 added enhancement New feature or request test removal labels Feb 22, 2023
@arbulu89 arbulu89 marked this pull request as ready for review February 22, 2023 16:28
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks good!

assets/js/components/ClustersList.test.jsx Show resolved Hide resolved
Copy link
Contributor

@dottorblaster dottorblaster 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 the huge contribution! Besides, I don't know if having the cypress tests ported as-they-are adds much value.

Also, I would like to have the removal of the cypress stuff in a subsequent PR.

About the testing of the filters inside the table test, I would like to test that purely without the router and the query string, and have the query-string-based test inside the overviews test, since it already requires the router wrapping. This way we can split the testing in two layers, one that is pure and one that tests the outcome in a more "e2e" fashion. These two layers will overlap a little but this way we increase coverage and robustness.

@CDimonaco what do you think?

assets/js/components/Table/Table.test.jsx Outdated Show resolved Hide resolved
assets/js/components/Table/Table.test.jsx Outdated Show resolved Hide resolved
assets/js/components/Table/Table.test.jsx Outdated Show resolved Hide resolved
assets/js/components/Table/Table.test.jsx Outdated Show resolved Hide resolved
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

I totally agree with all the comments made by @dottorblaster.

I think we will gain much value by splitting this process in separate prs, one for the pure table filtering, in which we test the table component and all the configuration, the filtering and the pure interactions with that component, one for the "integration" in other views and one for the removal.

Anyway thanks for the contribution, I think that splitting and addressing the comments made gains a lot of value and makes the review process more easy

@arbulu89
Copy link
Contributor Author

@CDimonaco @dottorblaster The other PR rebased, so this one ready to review!

@arbulu89 arbulu89 changed the title Jest table filter tests Jest table filter tests on the overview pages Feb 27, 2023
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Looks good

@arbulu89 arbulu89 merged commit 07d7b5a into main Mar 1, 2023
@arbulu89 arbulu89 deleted the jest-table-filter-tests branch March 1, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request removal test
Development

Successfully merging this pull request may close these issues.

None yet

4 participants