Skip to content

Add unit tests for keycloak service#252

Merged
a77med-Ibrahim merged 15 commits into
developfrom
Add-unit-tests-for-KeycloakService
Jun 30, 2026
Merged

Add unit tests for keycloak service#252
a77med-Ibrahim merged 15 commits into
developfrom
Add-unit-tests-for-KeycloakService

Conversation

@a77med-Ibrahim

@a77med-Ibrahim a77med-Ibrahim commented Jun 20, 2026

Copy link
Copy Markdown
Member

Summary

This PR improves Keycloak-related tests by separating application behavior from the Keycloak SDK integration layer.

KeycloakServiceTest now validates service behavior through a fake KeycloakGateway, while KeycloakGatewayImplTest verifies that the gateway interacts correctly with Keycloak HTTP endpoints.

Changes

  • Added a KeycloakGateway boundary for Keycloak operations.
  • Moved direct Keycloak SDK calls into KeycloakGatewayImpl.
  • Updated KeycloakService to use the gateway for:
    • user lookup
    • user creation
    • role assignment
    • password reset
    • reset-password email
    • password verification
  • Reworked KeycloakServiceTest to validate service behavior without mocking the full Keycloak SDK object graph.
  • Added KeycloakGatewayImplTest to cover the Keycloak adapter behavior against a mock Keycloak HTTP server.
  • Fixed Sonar issues related to:
    • closing Response with try-with-resources
    • avoiding multiple possibly-throwing calls inside exception assertion lambdas
    • removing the unused dummy assignment from password verification

Verification

./mvnw -pl app test
./mvnw -pl app test -Dtest=KeycloakGatewayImplTest,KeycloakServiceTest
./mvnw -pl app spotless:check

@ma-mirzaei

Copy link
Copy Markdown
Contributor

Please complete the documentation(fill description, assign the PR to yourself).
Set the target branch to the develop branch.

@a77med-Ibrahim a77med-Ibrahim self-assigned this Jun 22, 2026
@a77med-Ibrahim a77med-Ibrahim changed the base branch from main to develop June 22, 2026 10:53
@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Hi, I have completed the PR description, assigned the PR to myself, and changed the target branch to develop.

All checks have passed, and the PR is ready for review.
Thank you.

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

Tests are too implementation-coupled to Keycloak SDK. They should validate behavior, not mock the entire Keycloak object graph.

@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Hi Mohammed,

I have applied some changes to KeycloakService.java to separate the service logic from the Keycloak SDK implementation details. I introduced a small KeycloakGateway boundary and moved the direct Keycloak SDK calls into KeycloakGatewayImpl. This allows KeycloakServiceTest to focus on testing the expected service behavior and errors without mocking the full Keycloak SDK object graph.

All checks have passed, and the PR is ready for review.

Thank you

@ma-mirzaei ma-mirzaei self-requested a review June 23, 2026 09:32

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

This PR is intended to add test cases. Why are the Checkstyle configuration and suppressions being modified? Please keep unrelated build configuration changes out of this PR unless they are required for the tests and explain the reason.

@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Hi Mohammed,

I initially changedd the pom.xml while troubleshooting the test execution locally, because the first test approach was failing due to Mockito/Byte Buddy setup issues. After revising the tests to avoid mocking the full Keycloak SDK object graph, those build configuration changes were no longer needed.
I have now reverted the unrelated pom.xml changes, so the PR only keeps the changes needed for the KeycloakService tests and the SDK separation.

Please let me know if there are any other changes needed

Thank you

@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Hi Mohammed,

I noticed that you recently changed the status of this issue to Done, and the “Merge pull request” button is green on my side. However, I don’t see an approval from you on the pull request.

Should I go ahead and merge it, or should I wait for your approval first?

Thank you.

@ma-mirzaei

Copy link
Copy Markdown
Contributor

I checked the PR again, and when I checked the Sonar analyze, I found the issue
https://sonarcloud.io/project/issues?pullRequest=252&open=AZ71lBcOkXMtfYKNO-oz&id=vitruv-tools_methodologistUI-backend
Please fix it.
Also, you can check the Sonar analysis on your PR and resolve important issues

@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Hello Mohammed,

I have fixed the SonarQube issues under this pull request, and the PR is ready for review.

There is still one remaining SonarQube issue: “Remove this 'public' modifier”. However, the related file does not seem to exist on my local machine, so I am not sure how to resolve it from my side.

Could you please advise me on how I should handle this case?

Also, please let me know if there is anything else I should change, or if the PR is ready to be merged after your approval.

Thank you.

@ma-mirzaei

Copy link
Copy Markdown
Contributor

Whenever you want to review the project again, use the curly symbol near the reviewer’s name. The reviewer can then start reviewing it again.

@a77med-Ibrahim

a77med-Ibrahim commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

Hello Mohammed,

After the merge from Develop to this branch, 23 issues occurred on Sonar.

Should I resolve them or Merge to develop after review and work on vitruv-change?

Thank you

@ma-mirzaei

Copy link
Copy Markdown
Contributor

Just resolve the important issues that relate to your task's scope.
If you see a very important Sonar issue that does not relate to your scope, you can open a new issue card and work on it.

@a77med-Ibrahim

Copy link
Copy Markdown
Member Author

Okay, since all the new issues are not related to the main objective of this PR.
I could merge and create new issues for the other problems

I have the "merge pull request" button as green but I don't see your approval
Could you please confirm if this PR is ready to be merged or not?

Thank you Mohammed

@a77med-Ibrahim a77med-Ibrahim marked this pull request as draft June 30, 2026 12:38
@a77med-Ibrahim a77med-Ibrahim marked this pull request as ready for review June 30, 2026 12:38
@a77med-Ibrahim a77med-Ibrahim merged commit 52fbad2 into develop Jun 30, 2026
3 checks passed
@a77med-Ibrahim a77med-Ibrahim deleted the Add-unit-tests-for-KeycloakService branch June 30, 2026 13:08
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.

3 participants