Skip to content

test: Add identity provider integration tests#831

Open
mashawes wants to merge 1 commit into
openstack-experimental:mainfrom
mashawes:identity-integration-tests
Open

test: Add identity provider integration tests#831
mashawes wants to merge 1 commit into
openstack-experimental:mainfrom
mashawes:identity-integration-tests

Conversation

@mashawes

Copy link
Copy Markdown
Collaborator

Adds integration test coverage for the identity provider, following the existing test patterns and harness.

  • Group CRUD under tests/integration/src/identity/group/: create, get (including not-found), list (including filtering by domain), and delete.
  • User lifecycle: a delete test, a get_user_domain_id test, and a find_federated_user test (creating a user with a federated identity and looking it up by idp/unique id, plus the not-found case).
  • Group membership under tests/integration/src/identity/user_group/: remove and set, bulk add_users_to_groups and remove_user_from_groups, and the expiring variants (add, remove, remove-many, set), complementing the existing add/list coverage.
  • All tests use the existing harness (get_state, create_domain!, create_user!, create_group!) and the public provider API; no production code is changed. authenticate_by_password is exercised by the token integration tests.

Closes #449

Note: this contribution was developed with AI assistance.

@mashawes mashawes self-assigned this Jun 23, 2026
@mashawes
mashawes requested a review from gtema June 23, 2026 13:26

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

generally good, but we need some refinements:

  • add update tests as well
  • try to add some negative tests (adding user to non-existing group or vice versa, using duplicate names, invalid data, delete not existing resource, etc)

let domain = create_domain!(state)?;
let name = Uuid::new_v4().to_string();

let group = state

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.

let's please use "create_user!", "create_group!" like the tests/integration/src/identity/user_group/add.rs - we should be using this pattern more since it includes automatic cleanup of the resources created by the test what helps to fix test reliability. It is actually already present in some of the added tests.

use tracing_test::traced_test;
use uuid::Uuid;

use openstack_keystone::identity::IdentityApi;

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.

From now on this import is not necessary. Please remove it, but you actually should just run cargo check -p test_integration --tests to see warnings. Focus only in the files you add

@mashawes
mashawes requested a review from gtema June 29, 2026 13:20
@gtema
gtema force-pushed the identity-integration-tests branch from dd0b043 to 7241113 Compare July 3, 2026 16:05

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

since we now have DCO check which need to pass for all commits you are in unfortunate situation forcing you to either rewrite every single commit message , or, better, squash all changes to the single commit and put a proper commit message to it (you can do it with 'git rebase -i origin/main' - or it may be upstream/main). If you are not able to solve this (since this is a fork) you can follow a different approach and cherry-pick all 3 commits to the new branch (git cherry-pick -n 7241113, git cherry-pick -n .........). Another approach for not recreating the change is to do a git reset --soft $(git merge-base HEAD main) or just 3 times git reset --soft HEAD^1. This will undo commits while keeping the changes.

Which leaves the git rebase -i origin/main and pick the reword for every commit the easiest option.

Otherwise tests themselves looks fine

@mashawes
mashawes force-pushed the identity-integration-tests branch from 7241113 to fb3b586 Compare July 6, 2026 15:06
@gtema
gtema force-pushed the identity-integration-tests branch 2 times, most recently from 06bfd78 to 6982ef2 Compare July 7, 2026 09:36
@gtema
gtema enabled auto-merge (squash) July 7, 2026 10:03
@gtema
gtema force-pushed the identity-integration-tests branch from 6982ef2 to 333fa70 Compare July 7, 2026 11:25

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

Ouch, there was an error in workflows causing the test not to be actually running at all. Please fix errors and lonter complains

@ShJ-code

Copy link
Copy Markdown
Collaborator

I reviewed the CI error and found that all errors are reporting the same type mismatch issue: the tests added within this PR call the IdentityApi provider methods and pass &state as their first argument. However, probably due to the upgrade of the API, every one of those methods takes an execution context as its first parameter. You might want to use the constructor fn internal from ExecutionContext (within crates/core/src/auth.ts) since the integration tests do not have authenticated caller. Specifically, the context could be built once in each test before passed in.

Signed-off-by: Mohammed <mashawes@andrew.cmu.edu>
@gtema
gtema force-pushed the identity-integration-tests branch from 333fa70 to 2e03185 Compare July 10, 2026 09:48
@gtema

gtema commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

I reviewed the CI error and found that all errors are reporting the same type mismatch issue: the tests added within this PR call the IdentityApi provider methods and pass &state as their first argument. However, probably due to the upgrade of the API, every one of those methods takes an execution context as its first parameter. You might want to use the constructor fn internal from ExecutionContext (within crates/core/src/auth.ts) since the integration tests do not have authenticated caller. Specifically, the context could be built once in each test before passed in.

yes, there was a bigger restructure. Please adapt to how other tests initialize the ExecutionContext

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement integration tests for identity provider

3 participants