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

Issue 330 - Add tests for Hosts page #331

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

mreynolds389
Copy link
Collaborator

Add tests for adding/deleting hosts, and automember rebuild

Since we don't know the host name I created "partial" functions for elements that "start with" a value, instead of searching for the entire value.

Fixes: #330

Copy link
Collaborator

@carma12 carma12 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 PR. Please check the comments below:

tests/features/steps/common.ts Outdated Show resolved Hide resolved
Given I am logged in as "Administrator"
Given I am on "hosts" page

Scenario Outline: Add a new host
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to add more tests to check the different addition of new hosts (as they're sifferent ways/options to do so). I.e.:

  • Partial + no checkbox selected (done ✅ )
  • Complete (with full data) + no checkbox selected
  • Complete (with full data) + 'Force' checkbox
  • Partial + 'Generate OTP' / 'Suppress processing of membership attributes' checkbox

tests/features/host_handling.feature Outdated Show resolved Hide resolved
tests/features/host_handling.feature Show resolved Hide resolved
@mreynolds389 mreynolds389 force-pushed the test_hosts branch 3 times, most recently from 7032a03 to 138c120 Compare April 16, 2024 13:15
@mreynolds389
Copy link
Collaborator Author

@carma12 @miskopo all changes made, this is ready for review again. Thanks!

@mreynolds389
Copy link
Collaborator Author

Also I am very open to renaming things in this PR. I use "partial" entry a lot. Couldn't come up with anything better.

I also use "modal" for describing some functions in common.ts, but that might not be entirely correct. Maybe it applies outside of modals?

Copy link
Member

@miskopo miskopo left a comment

Choose a reason for hiding this comment

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

Hello @mreynolds389 , thank you for your contribution, please see the in-line comments.

src/components/modals/AddHost.tsx Show resolved Hide resolved
src/components/modals/AddHost.tsx Show resolved Hide resolved
src/components/modals/AddHost.tsx Show resolved Hide resolved
src/components/modals/AddHost.tsx Show resolved Hide resolved
tests/features/steps/common.ts Show resolved Hide resolved
tests/features/steps/common.ts Show resolved Hide resolved
@miskopo
Copy link
Member

miskopo commented Apr 17, 2024

Also, when labels' for attributes are properly set, it would be possible to use function

"I type in the field {string} text {string}", ...

from common.ts, line 108

Copy link
Member

@miskopo miskopo left a comment

Choose a reason for hiding this comment

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

As mentioned in the comments, there are some things that will need to be addressed later, however, I approve this PR.
Thank you for your contribution.

@miskopo miskopo added ack Pull Request approved, it can be merged tests PR related to testing labels Apr 19, 2024
Add tests for adding/deleting hosts, and automember rebuild

Since we don't know the host name I created "partial" functions
for elements that "start with" a value, instead of searching for
the entire value.

Fixes: freeipa#330

Signed-off-by: Mark Reynolds <[email protected]>
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

LGTM.

@mreynolds389 mreynolds389 merged commit c658b02 into freeipa:main Apr 19, 2024
2 of 3 checks passed
@mreynolds389 mreynolds389 deleted the test_hosts branch April 19, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, it can be merged tests PR related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for Hosts page
3 participants