-
Notifications
You must be signed in to change notification settings - Fork 75
Add unit tests for doPreUpdateApplication method #498
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
base: main
Are you sure you want to change the base?
Add unit tests for doPreUpdateApplication method #498
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #498 +/- ##
============================================
+ Coverage 57.58% 57.69% +0.10%
- Complexity 1740 1759 +19
============================================
Files 177 177
Lines 9688 9748 +60
Branches 1357 1378 +21
============================================
+ Hits 5579 5624 +45
- Misses 3642 3653 +11
- Partials 467 471 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds new unit tests for the doPreUpdateApplication method to ensure its correct behavior for different audiences and associated roles scenarios.
- Added a new data provider for testing associated roles.
- Introduced tests for both organization and application audiences.
- Verified the updates to thread-local properties after processing the update.
Comments suppressed due to low confidence (1)
components/org.wso2.carbon.identity.organization.management.handler/src/test/java/org/wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java:358
- [nitpick] Consider using the constant ORGANIZATION_AUD instead of the literal string "ORGANIZATION" to maintain consistency across the test code.
associatedRolesConfig.setAllowedAudience("ORGANIZATION");
3e0d552
to
a8dd9f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces new unit tests for the doPreUpdateApplication method to validate role audience updates.
- Added a new data provider for testing associated roles scenarios.
- Introduced two new unit test methods to cover both organization and application audience scenarios.
Comments suppressed due to low confidence (1)
components/org.wso2.carbon.identity.organization.management.handler/src/test/java/org/wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java:241
- [nitpick] Consider renaming 'associatedRolesConfig02' to a more descriptive name like 'orgAudienceRolesConfig' to improve code clarity.
AssociatedRolesConfig associatedRolesConfig02 = new AssociatedRolesConfig();
93fb7a8
to
56ea9bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds unit tests for the doPreUpdateApplication method to validate role audience updates related to organization and application audiences.
- Introduces new test cases including tests for organization audience and associated roles.
- Adds a data provider for scenarios involving different role audiences.
Comments suppressed due to low confidence (1)
components/org.wso2.carbon.identity.organization.management.handler/src/test/java/org/wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java:226
- [nitpick] Consider renaming the data provider method to a shorter, clearer name (e.g., 'updateAppRolesDataProvider') to improve code readability.
@DataProvider(name = "testDoPreUpdateAppWithAssociatedRolesDataProvider")
...wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java
Outdated
Show resolved
Hide resolved
56ea9bb
to
fc77363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces additional unit tests to validate the behavior of the doPreUpdateApplication method.
- Added new test cases and a data provider to cover scenarios with associated roles and different audience types.
- Introduced new constants and necessary imports to support the unit tests.
Comments suppressed due to low confidence (2)
components/org.wso2.carbon.identity.organization.management.handler/src/test/java/org/wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java:408
- Consider clearing or isolating IdentityUtil.threadLocalProperties before and after tests to ensure test isolation and prevent interference between test cases.
Object finalUpdatedAssociatedRolesList = IdentityUtil.threadLocalProperties.get().get(ADDED_ORGANIZATION_AUDIENCE_ROLES);
components/org.wso2.carbon.identity.organization.management.handler/src/test/java/org/wso2/carbon/identity/organization/management/handler/listener/SharedRoleMgtListenerTest.java:243
- [nitpick] If the test scenario expects roles to be present in the organization audience configuration, consider initializing the roles array in orgAudienceAssociatedRolesConfig for clarity and consistency.
updatedOrgAudienceServiceProvider.setAssociatedRolesConfig(orgAudienceAssociatedRolesConfig);
Purpose