Skip to content

Integration tests: Add new test for privileges main page#1135

Merged
veronnicka merged 1 commit into
freeipa:mainfrom
veronnicka:intergation-test-privilege
Jul 16, 2026
Merged

Integration tests: Add new test for privileges main page#1135
veronnicka merged 1 commit into
freeipa:mainfrom
veronnicka:intergation-test-privilege

Conversation

@veronnicka

@veronnicka veronnicka commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Followed hbac_service.feature which main page is very similar

This PR is built on top of #1134 , can be reviewed but we will have to wait for the rebase for the checks to pass

Summary by Sourcery

Add integration tests for privileges main page covering privilege creation, search, deletion, bulk deletion, and cancel flows.

New Features:

  • Introduce a Cypress Cucumber feature spec for privileges CRUD operations on the main page.
  • Add step definitions to seed and delete privileges via IPA commands for test setup and cleanup.

Tests:

  • Add end-to-end scenarios for adding, searching, deleting single and multiple privileges, and cancelling privilege creation on the privileges page.

@veronnicka veronnicka added the needs-review This PR is waiting on a review label Jul 6, 2026

@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.

Hey - I've left some high level feedback:

  • There are multiple identical @seed scenarios creating the same privileges (e.g., a_privilege1, a_privilege2); consider consolidating these into a shared setup (Background or a single seed scenario) to reduce duplication and make the test flow clearer.
  • The cleanup steps only explicitly remove a_privilege1 and a_privilege2; if a_privilege3 or other test data can persist beyond their scenarios, it may be worth adding corresponding cleanup steps to keep the test environment consistent between runs.
  • In privileges.ts, the cy.ipa calls are fire-and-forget; if these setup/teardown steps need to be chained with other Cypress commands, consider returning the cy.ipa call from the step definitions to integrate properly with Cypress’s command queue.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- There are multiple identical `@seed` scenarios creating the same privileges (e.g., `a_privilege1`, `a_privilege2`); consider consolidating these into a shared setup (Background or a single seed scenario) to reduce duplication and make the test flow clearer.
- The cleanup steps only explicitly remove `a_privilege1` and `a_privilege2`; if `a_privilege3` or other test data can persist beyond their scenarios, it may be worth adding corresponding cleanup steps to keep the test environment consistent between runs.
- In `privileges.ts`, the `cy.ipa` calls are fire-and-forget; if these setup/teardown steps need to be chained with other Cypress commands, consider returning the `cy.ipa` call from the step definitions to integrate properly with Cypress’s command queue.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@veronnicka veronnicka added Needs Rebase needs-review This PR is waiting on a review and removed needs-review This PR is waiting on a review labels Jul 7, 2026

@carma12 carma12 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.

Haven't fully tried it locally yet because it requires to rebase the changes from #1134 , but found something that can be addressed. See comment below:

Comment thread cypress/e2e/privileges/privileges.feature
@carma12 carma12 removed the needs-review This PR is waiting on a review label Jul 7, 2026
@carma12

carma12 commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

@veronnicka - Removed the needs-review tag on this PR to leave the needs-rebase only as reminder (we can add the other later when everything is updated and rebased).

Followed hbac_service.feature which main page is very similar

Signed-off-by: Veronika Miticka <vmiticka@redhat.com>
@veronnicka
veronnicka force-pushed the intergation-test-privilege branch from 681616b to 7e349e8 Compare July 15, 2026 08:37
@veronnicka veronnicka added needs-review This PR is waiting on a review and removed Needs Rebase labels Jul 15, 2026
@veronnicka

Copy link
Copy Markdown
Contributor Author

The code is rebased and the checks are passing. Please have a look @carma12

@carma12 carma12 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.

Maybe we can add a new test that checks the addition of a new Privilege with description, but it's completely fine if we don't include it. Apart from that, LGTM.

@veronnicka

Copy link
Copy Markdown
Contributor Author

Maybe we can add a new test that checks the addition of a new Privilege with description, but it's completely fine if we don't include it. Apart from that, LGTM.

The first tests adds a privilige with a description. Is that what you meant?

@veronnicka
veronnicka merged commit b1a7e49 into freeipa:main Jul 16, 2026
7 of 9 checks passed
@veronnicka
veronnicka deleted the intergation-test-privilege branch July 16, 2026 09:17
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.

2 participants