Skip to content

[Integration tests] 'Roles' pages#1133

Merged
carma12 merged 1 commit into
freeipa:mainfrom
carma12:roles-integration-tests
Jul 14, 2026
Merged

[Integration tests] 'Roles' pages#1133
carma12 merged 1 commit into
freeipa:mainfrom
carma12:roles-integration-tests

Conversation

@carma12

@carma12 carma12 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

The integration tests must test the functionality
of the Roles' following pages:

  • Main page
  • 'Settings' page
  • 'Members' page
  • 'Privileges' page

This PR has been created on the top of #1132 , so it must be merged first.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @carma12, your pull request is larger than the review limit of 150000 diff characters

@b3lix b3lix left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

@carma12

carma12 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

I added a note in the description: the #1132 PR needs to be merged first before rebasing this one.

@b3lix

b3lix commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

I added a note in the description: the #1132 PR needs to be merged first before rebasing this one.

I overlooked, I see it now, thanks for info ... looking into 1132 already.

@carma12
carma12 requested a review from b3lix July 6, 2026 09:19

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, went through the steps, just have a comment regarding test scenarios with the state of the buttons. As I recall we didnt test this in other tests, it is a behaviour that is the same across the subpages. At least this is my understanding, please correct me if im wrong.

Comment thread cypress/e2e/common/ui/dual_list.ts
Comment thread cypress/e2e/roles/roles_privileges.feature Outdated
Given role "button_state_role" exists

@test
Scenario: Save and Revert buttons are disabled when no changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as my other comment.

@carma12 carma12 Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I considered leaving this one here (although we don't have it in other 'Settings'-related tests) because I have seen that sometimes those buttons tend to not be enabled/disabled properly when a change is done. Not really necessary or critical, but it is open to discussion whether we want to keep it or not.

Just to provide more context: All 'Settings' pages have three main buttons: 'Refresh', 'Revert', and 'Save'. By default, the 'Revert' and 'Save' buttons are disabled. When something is modified in the 'Settings' page (e.g., I change the description field or any other field), those buttons are enabled and ready to be clicked. This is the only scenario on which we want to have those buttons enabled, and not when no modification has been done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Im fine with keeping it then.

@carma12
carma12 force-pushed the roles-integration-tests branch 2 times, most recently from 5f0115c to c4fa964 Compare July 10, 2026 13:46
@carma12 carma12 added needs-review This PR is waiting on a review and removed Needs Rebase labels Jul 10, 2026

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good, feel free to merge after the rebase and when the checks pass.

Given role "button_state_role" exists

@test
Scenario: Save and Revert buttons are disabled when no changes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Im fine with keeping it then.

The integration tests must test the functionality
of the Roles' following pages:
- Main page
- 'Settings' page
- 'Members' page
- 'Privileges' page

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Carla Martinez <carlmart@redhat.com>
@carma12
carma12 force-pushed the roles-integration-tests branch from c4fa964 to 3d5c524 Compare July 14, 2026 08:33
@carma12

carma12 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased and waiting for the tests to finish before merging...

@carma12
carma12 merged commit a360610 into freeipa:main Jul 14, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review This PR is waiting on a review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants