-
Notifications
You must be signed in to change notification settings - Fork 554
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
Introduce the fragment app check when adding the app role association #6185
Introduce the fragment app check when adding the app role association #6185
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6185 +/- ##
============================================
+ Coverage 40.93% 45.56% +4.63%
+ Complexity 15735 14108 -1627
============================================
Files 1812 1631 -181
Lines 126414 100847 -25567
Branches 22448 16960 -5488
============================================
- Hits 51746 45951 -5795
+ Misses 67057 48194 -18863
+ Partials 7611 6702 -909
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...ole.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
Show resolved
Hide resolved
...ole.v2.mgt.core/src/main/java/org/wso2/carbon/identity/role/v2/mgt/core/dao/RoleDAOImpl.java
Outdated
Show resolved
Hide resolved
2e2fe44
to
9804566
Compare
Add unit test to cover changed code |
374239f
to
7e50459
Compare
...in/java/org/wso2/carbon/identity/application/mgt/listener/DefaultRoleManagementListener.java
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
833da37
to
0ef30a7
Compare
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
// Remove the mocked threadLocalProperties. | ||
Map<String, Object> threadLocalProperties = new HashMap<>(); | ||
IdentityUtil.threadLocalProperties.set(threadLocalProperties); | ||
} |
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.
Here can't we validate whether the thread local property is removed ? Then that will be also an assertion as well?
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.
No we cant because the thread local property will be added from a listener and the listeners are not executing when the test method is running.
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.
Yes specifically adding the property before test method is required,
but when executing the test method "private boolean isFragmentApp()" method should be invoked in this case and it should remove the thread local right?
Given that is the expectation, here we can validate that thread local property has been removed at the end
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.
Assertion added to check whether the thread local property is cleared
0ef30a7
to
9969ce2
Compare
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Outdated
Show resolved
Hide resolved
1978f96
to
b268a5e
Compare
...rc/test/java/org/wso2/carbon/identity/application/mgt/DefaultRoleManagementListenerTest.java
Show resolved
Hide resolved
b268a5e
to
03e663d
Compare
|
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/12367206077
Proposed changes in this pull request