Conversation
…om UMA policy endpoint Conflicts: testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java (cherry picked from commit 13923a7)
mhajas
left a comment
There was a problem hiding this comment.
@pedroigor @stianst I would really like to have my suggestions (https://github.com/keycloak/keycloak-prod/pull/188#pullrequestreview-289209210) incorporated in this PR. The reason is that at the moment pipelines are not testing any SCRIPT based features, because we are not enabling it anywhere.
I was thinking about this and I can imagine we could introduce an annotation like: @EnableFeature(UPLOAD_SCRIPTS) and make arquillian enable it (including restarting of container so that providers would be loaded). We need something similar to this: #6387. This way we could get rid of all isFeatureEnabled assumptions and all tests will be running with correct features enabled by default.
The second suggestion would be a little bit harder to implement because we have a lot of tests which are using Javascript policies, maybe we can postpone this one for later.
WDYT?
|
Sorry I checked it and DeployedScriptPolicyTest is running because it doesn't need SCRIPTS feature enabled. However DeployedScriptAuthenticatorTest is not running anywhere. |
|
@mhajas Do you mean these two points ?
|
|
@pedroigor yes |
|
I'm OK to proceed with your suggestions. Except for #2. We have quite a few tests that rely on JS policies to perform additional tests on how policies are evaluated and how we are covering different scenarios. It would require some more time to identify all places and, carefully, either remove any unnecessary JS usage or replace it with a corresponding file to be deployed. Considering the priorities we have, I'm afraid I won't be able to step into this. So I would forward the decision on how to proceed to @stianst. Regarding the DeployedScriptAuthenticatorTest, we definitely need to run it. What about the protocol mapper tests, is it running ? |
|
As I said in my first comment, I am okay with excluding this for now. No ScriptAuthenticators and ProtocolMappers are not tested at the moment. I just did manual testing for your PR. The solution I proposed in the first comment would solve this problem. |
|
@mhajas, shall we include the "enableFeature restart auth-server" in this PR or a separated one ? In addition to that, would that increase test execution time? |
|
I am not sure whether it should be in this PR since it is quite big already. On the other hand this extension is a must have for me. I am ok with both options if the second Pr will be created asap. Yes it will. That is why I am not sure we need to use "EnableFeature with restart" also for UPLOAD_SCRIPTS feature, since it is not necessary to restart the server for enabling it. Maybe we can have option restartContainer in annotation (defaulting to true) and just disable it for UPLOAD_FEATURE feature. Btw. do you want to implement this? If you have no time for this I can do this for you. |
|
@mhajas I'm not the proud guy I used to be, so any help is more than appreciated :) Besides, you have more knowledge of test suite than me. A follow-up PR sounds better for me too. |
|
@pedroigor Agree, I created an issue and I will start working on it tomorrow. |
ee2ce90 to
b5aac85
Compare
No description provided.