Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 0.11.0
    • None
    • None

    Description

      At the moment we use Powermock in various modules. Since Powermock is not supported in JUnit5, we should remove the dependency.

      In most tests, we mock environment variables with Powermock.

      Attachments

        Activity

          huage1994 Guanhua LI added a comment -

          Hi  Philipp Dallig,

            I think this ticket is very neccesary.   I'm interested in it.

            Do you have started to deal with this ticket?

            

          huage1994 Guanhua LI added a comment - Hi  Philipp Dallig,   I think this ticket is very neccesary.   I'm interested in it.   Do you have started to deal with this ticket?   
          Reamer Philipp Dallig added a comment -

          I played around with it a bit in the Zeppelin plugins. Here in particular in the Docker plugin.

          I do not have a good solution. Have looked at extensions of Mockito in this context, but do not know if this is the right and sustainable way. Currently I tend to take off the environment variables as configuration into the ZeppelinConfiguration and mock the ZeppelinConfiguration.

          Reamer Philipp Dallig added a comment - I played around with it a bit in the Zeppelin plugins. Here in particular in the Docker plugin. I do not have a good solution. Have looked at extensions of Mockito in this context, but do not know if this is the right and sustainable way. Currently I tend to take off the environment variables as configuration into the ZeppelinConfiguration and mock the ZeppelinConfiguration.
          huage1994 Guanhua LI added a comment -

          huage1994 Guanhua LI added a comment -
          githubbot ASF GitHub Bot added a comment -

          Reamer opened a new pull request, #4519:
          URL: https://github.com/apache/zeppelin/pull/4519

              1. What is this PR for?
                [Powermock](https://github.com/powermock/powermock/issues/929) is not supported in JUnit5. We should not be dependent on Powermock. This PR removes the Powermock dependency from the Docker plugin. It also makes some refactoring.

          Instead of using environment variables directly, this possibility of configuration is moved to ZeppelinConfiguration. In order not to break the naming scheme in ZeppelinConfiguration, the environment variables that were previously accessed must be changed.

          `CONTAINER_SPARK_HOME` -> `ZEPPELIN_DOCKER_CONTAINER_SPARK_HOME`
          `UPLOAD_LOCAL_LIB_TO_CONTAINTER` -> `ZEPPELIN_DOCKER_UPLOAD_LOCAL_LIB_TO_CONTAINTER`
          `DOCKER_HOST` -> `ZEPPELIN_DOCKER_HOST`
          `DOCKER_TIME_ZONE` -> `ZEPPELIN_DOCKER_TIME_ZONE`

          Except for DOCKER_TIME_ZONE, I have not found any references in the documentation or other files.

              1. What type of PR is it?

          Refactoring

              1. What is the Jira issue?
              1. How should this be tested?
          • CI
              1. Questions:
          • Does the licenses files need to update? No
          • Is there breaking changes for older versions? Yes
          • Does this needs documentation? No
          githubbot ASF GitHub Bot added a comment - Reamer opened a new pull request, #4519: URL: https://github.com/apache/zeppelin/pull/4519 What is this PR for? [Powermock] ( https://github.com/powermock/powermock/issues/929 ) is not supported in JUnit5. We should not be dependent on Powermock. This PR removes the Powermock dependency from the Docker plugin. It also makes some refactoring. Instead of using environment variables directly, this possibility of configuration is moved to ZeppelinConfiguration. In order not to break the naming scheme in ZeppelinConfiguration, the environment variables that were previously accessed must be changed. `CONTAINER_SPARK_HOME` -> `ZEPPELIN_DOCKER_CONTAINER_SPARK_HOME` `UPLOAD_LOCAL_LIB_TO_CONTAINTER` -> `ZEPPELIN_DOCKER_UPLOAD_LOCAL_LIB_TO_CONTAINTER` `DOCKER_HOST` -> `ZEPPELIN_DOCKER_HOST` `DOCKER_TIME_ZONE` -> `ZEPPELIN_DOCKER_TIME_ZONE` Except for DOCKER_TIME_ZONE, I have not found any references in the documentation or other files. What type of PR is it? Refactoring What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-5855 How should this be tested? CI Questions: Does the licenses files need to update? No Is there breaking changes for older versions? Yes Does this needs documentation? No
          githubbot ASF GitHub Bot added a comment -

          Reamer commented on PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519#issuecomment-1404800082

          I need a review.

          githubbot ASF GitHub Bot added a comment - Reamer commented on PR #4519: URL: https://github.com/apache/zeppelin/pull/4519#issuecomment-1404800082 I need a review.
          githubbot ASF GitHub Bot added a comment -

          jongyoul commented on code in PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087820276

          ##########
          zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java:
          ##########
          @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException

          { throw new IOException(e.getMessage()); }

          catch (InterruptedException e)

          { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); }

          long startTime = System.currentTimeMillis();

          // wait until interpreter send dockerStarted message through thrift rpc
          synchronized (dockerStarted) {

          • if (!dockerStarted.get()) {
            + while (!dockerStarted.get()) {

          Review Comment:
          It sometimes can make an infinite loop even if docker is really stable and guarantee to return in time. How about maximum retry or timeout like the below code? WDYT?

          githubbot ASF GitHub Bot added a comment - jongyoul commented on code in PR #4519: URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087820276 ########## zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java: ########## @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException { throw new IOException(e.getMessage()); } catch (InterruptedException e) { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); } long startTime = System.currentTimeMillis(); // wait until interpreter send dockerStarted message through thrift rpc synchronized (dockerStarted) { if (!dockerStarted.get()) { + while (!dockerStarted.get()) { Review Comment: It sometimes can make an infinite loop even if docker is really stable and guarantee to return in time. How about maximum retry or timeout like the below code? WDYT?
          githubbot ASF GitHub Bot added a comment -

          jongyoul commented on code in PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087820276

          ##########
          zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java:
          ##########
          @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException

          { throw new IOException(e.getMessage()); }

          catch (InterruptedException e)

          { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); }

          long startTime = System.currentTimeMillis();

          // wait until interpreter send dockerStarted message through thrift rpc
          synchronized (dockerStarted) {

          • if (!dockerStarted.get()) {
            + while (!dockerStarted.get()) {

          Review Comment:
          It sometimes can make an infinite loop even if docker is really stable and guarantee to return in time. How about having maximum retry or timeout like the below code? WDYT?

          githubbot ASF GitHub Bot added a comment - jongyoul commented on code in PR #4519: URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087820276 ########## zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java: ########## @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException { throw new IOException(e.getMessage()); } catch (InterruptedException e) { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); } long startTime = System.currentTimeMillis(); // wait until interpreter send dockerStarted message through thrift rpc synchronized (dockerStarted) { if (!dockerStarted.get()) { + while (!dockerStarted.get()) { Review Comment: It sometimes can make an infinite loop even if docker is really stable and guarantee to return in time. How about having maximum retry or timeout like the below code? WDYT?
          githubbot ASF GitHub Bot added a comment -

          Reamer commented on code in PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087924889

          ##########
          zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java:
          ##########
          @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException

          { throw new IOException(e.getMessage()); }

          catch (InterruptedException e)

          { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); }

          long startTime = System.currentTimeMillis();

          // wait until interpreter send dockerStarted message through thrift rpc
          synchronized (dockerStarted) {

          • if (!dockerStarted.get()) {
            + while (!dockerStarted.get()) {

          Review Comment:
          Your right, I will change this.

          githubbot ASF GitHub Bot added a comment - Reamer commented on code in PR #4519: URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1087924889 ########## zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java: ########## @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException { throw new IOException(e.getMessage()); } catch (InterruptedException e) { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); } long startTime = System.currentTimeMillis(); // wait until interpreter send dockerStarted message through thrift rpc synchronized (dockerStarted) { if (!dockerStarted.get()) { + while (!dockerStarted.get()) { Review Comment: Your right, I will change this.
          githubbot ASF GitHub Bot added a comment -

          Reamer commented on code in PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1088763586

          ##########
          zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java:
          ##########
          @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException

          { throw new IOException(e.getMessage()); }

          catch (InterruptedException e)

          { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); }

          long startTime = System.currentTimeMillis();

          // wait until interpreter send dockerStarted message through thrift rpc
          synchronized (dockerStarted) {

          • if (!dockerStarted.get()) {
            + while (!dockerStarted.get()) {

          Review Comment:
          I have adapted the corresponding passage. Please look over it again.

          githubbot ASF GitHub Bot added a comment - Reamer commented on code in PR #4519: URL: https://github.com/apache/zeppelin/pull/4519#discussion_r1088763586 ########## zeppelin-plugins/launcher/docker/src/main/java/org/apache/zeppelin/interpreter/launcher/DockerInterpreterProcess.java: ########## @@ -242,18 +233,22 @@ public void progress(ProgressMessage message) throws DockerException { throw new IOException(e.getMessage()); } catch (InterruptedException e) { LOGGER.error(e.getMessage(), e); + // Restore interrupted state... + Thread.currentThread().interrupt(); throw new IOException(e.getMessage()); } long startTime = System.currentTimeMillis(); // wait until interpreter send dockerStarted message through thrift rpc synchronized (dockerStarted) { if (!dockerStarted.get()) { + while (!dockerStarted.get()) { Review Comment: I have adapted the corresponding passage. Please look over it again.
          githubbot ASF GitHub Bot added a comment -

          jongyoul merged PR #4519:
          URL: https://github.com/apache/zeppelin/pull/4519

          githubbot ASF GitHub Bot added a comment - jongyoul merged PR #4519: URL: https://github.com/apache/zeppelin/pull/4519
          Reamer Philipp Dallig added a comment - Closed via https://github.com/apache/zeppelin/pull/4519
          Reamer Philipp Dallig added a comment -

          I was too fast powermock is still used in other modules.

          Reamer Philipp Dallig added a comment - I was too fast powermock is still used in other modules.
          githubbot ASF GitHub Bot added a comment -

          Reamer opened a new pull request, #4580:
          URL: https://github.com/apache/zeppelin/pull/4580

              1. What is this PR for?
                This PR removes the use of Powermock in further tests.
              1. What type of PR is it?
          • Refactoring
              1. What is the Jira issue?
              1. How should this be tested?
          • CI
              1. Questions:
          • Does the license files need to update? No
          • Is there breaking changes for older versions? No
          • Does this needs documentation? No
          githubbot ASF GitHub Bot added a comment - Reamer opened a new pull request, #4580: URL: https://github.com/apache/zeppelin/pull/4580 What is this PR for? This PR removes the use of Powermock in further tests. What type of PR is it? Refactoring What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-5855 How should this be tested? CI Questions: Does the license files need to update? No Is there breaking changes for older versions? No Does this needs documentation? No
          githubbot ASF GitHub Bot added a comment -

          Reamer commented on PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580#issuecomment-1501121841

          I need a review.

          githubbot ASF GitHub Bot added a comment - Reamer commented on PR #4580: URL: https://github.com/apache/zeppelin/pull/4580#issuecomment-1501121841 I need a review.
          githubbot ASF GitHub Bot added a comment -

          huage1994 commented on code in PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1161640522

          ##########
          alluxio/src/test/java/org/apache/zeppelin/alluxio/AlluxioInterpreterTest.java:
          ##########
          @@ -46,26 +42,32 @@
          import org.apache.zeppelin.interpreter.InterpreterResult;
          import org.apache.zeppelin.interpreter.InterpreterResult.Code;
          import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion;
          +import org.junit.jupiter.api.AfterEach;
          +import org.junit.jupiter.api.BeforeEach;
          +import org.junit.jupiter.api.Test;

          import static alluxio.cli.fs.command.CountCommand.COUNT_FORMAT;
          -import static org.junit.Assert.assertEquals;
          +import static org.junit.jupiter.api.Assertions.assertArrayEquals;
          +import static org.junit.jupiter.api.Assertions.assertEquals;
          +import static org.junit.jupiter.api.Assertions.assertNotEquals;
          +import static org.junit.jupiter.api.Assertions.assertTrue;

          -public class AlluxioInterpreterTest {

          Review Comment:
          `public` before `class` should not be removed

          githubbot ASF GitHub Bot added a comment - huage1994 commented on code in PR #4580: URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1161640522 ########## alluxio/src/test/java/org/apache/zeppelin/alluxio/AlluxioInterpreterTest.java: ########## @@ -46,26 +42,32 @@ import org.apache.zeppelin.interpreter.InterpreterResult; import org.apache.zeppelin.interpreter.InterpreterResult.Code; import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import static alluxio.cli.fs.command.CountCommand.COUNT_FORMAT; -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; -public class AlluxioInterpreterTest { Review Comment: `public` before `class` should not be removed
          githubbot ASF GitHub Bot added a comment -

          Reamer commented on code in PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1163956686

          ##########
          alluxio/src/test/java/org/apache/zeppelin/alluxio/AlluxioInterpreterTest.java:
          ##########
          @@ -46,26 +42,32 @@
          import org.apache.zeppelin.interpreter.InterpreterResult;
          import org.apache.zeppelin.interpreter.InterpreterResult.Code;
          import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion;
          +import org.junit.jupiter.api.AfterEach;
          +import org.junit.jupiter.api.BeforeEach;
          +import org.junit.jupiter.api.Test;

          import static alluxio.cli.fs.command.CountCommand.COUNT_FORMAT;
          -import static org.junit.Assert.assertEquals;
          +import static org.junit.jupiter.api.Assertions.assertArrayEquals;
          +import static org.junit.jupiter.api.Assertions.assertEquals;
          +import static org.junit.jupiter.api.Assertions.assertNotEquals;
          +import static org.junit.jupiter.api.Assertions.assertTrue;

          -public class AlluxioInterpreterTest {

          Review Comment:
          Well seen. I have adjusted it.

          githubbot ASF GitHub Bot added a comment - Reamer commented on code in PR #4580: URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1163956686 ########## alluxio/src/test/java/org/apache/zeppelin/alluxio/AlluxioInterpreterTest.java: ########## @@ -46,26 +42,32 @@ import org.apache.zeppelin.interpreter.InterpreterResult; import org.apache.zeppelin.interpreter.InterpreterResult.Code; import org.apache.zeppelin.interpreter.thrift.InterpreterCompletion; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import static alluxio.cli.fs.command.CountCommand.COUNT_FORMAT; -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; -public class AlluxioInterpreterTest { Review Comment: Well seen. I have adjusted it.
          githubbot ASF GitHub Bot added a comment -

          jongyoul commented on code in PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1164411099

          ##########
          zeppelin-server/src/test/java/org/apache/zeppelin/service/shiro/AbstractShiroTest.java:
          ##########
          @@ -0,0 +1,94 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.zeppelin.service.shiro;
          +
          +import org.apache.shiro.SecurityUtils;
          +import org.apache.shiro.UnavailableSecurityManagerException;
          +import org.apache.shiro.mgt.SecurityManager;
          +import org.apache.shiro.subject.Subject;
          +import org.apache.shiro.subject.support.SubjectThreadState;
          +import org.apache.shiro.util.LifecycleUtils;
          +import org.apache.shiro.util.ThreadState;
          +import org.junit.AfterClass;
          +
          +/**
          + * Abstract test case enabling Shiro in test environments.
          + * Copy&Paste from https://shiro.apache.org/testing.html
          + */
          +public abstract class AbstractShiroTest {
          +
          + private static ThreadState subjectThreadState;
          +
          + public AbstractShiroTest()

          { + }

          +
          + /**
          + * Allows subclasses to set the currently executing

          {@link Subject}

          instance.
          + *
          + * @param subject the Subject instance
          + */
          + protected void setSubject(Subject subject)

          { + clearSubject(); + subjectThreadState = createThreadState(subject); + subjectThreadState.bind(); + }

          +
          + protected Subject getSubject()

          { + return SecurityUtils.getSubject(); + }

          +
          + protected ThreadState createThreadState(Subject subject)

          { + return new SubjectThreadState(subject); + }

          +
          + /**
          + * Clears Shiro's thread state, ensuring the thread remains clean for future test execution.
          + */
          + protected void clearSubject()

          { + doClearSubject(); + }

          +
          + private static void doClearSubject() {
          + if (subjectThreadState != null)

          { + subjectThreadState.clear(); + subjectThreadState = null; + }

          + }
          +
          + protected static void setSecurityManager(SecurityManager securityManager)

          { + SecurityUtils.setSecurityManager(securityManager); + }

          +
          + protected static SecurityManager getSecurityManager()

          { + return SecurityUtils.getSecurityManager(); + }

          +
          + @AfterClass
          + public static void tearDownShiro() {
          + doClearSubject();
          + try

          { + SecurityManager securityManager = getSecurityManager(); + LifecycleUtils.destroy(securityManager); + }

          catch (UnavailableSecurityManagerException e)

          { + // we don't care about this when cleaning up the test environment + // (for example, maybe the subclass is a unit test and it didn't + // need a SecurityManager instance because it was using only + // mock Subject instances) + }

          + setSecurityManager(null);
          + }
          +}

          Review Comment:
          nit: add a new line

          githubbot ASF GitHub Bot added a comment - jongyoul commented on code in PR #4580: URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1164411099 ########## zeppelin-server/src/test/java/org/apache/zeppelin/service/shiro/AbstractShiroTest.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.zeppelin.service.shiro; + +import org.apache.shiro.SecurityUtils; +import org.apache.shiro.UnavailableSecurityManagerException; +import org.apache.shiro.mgt.SecurityManager; +import org.apache.shiro.subject.Subject; +import org.apache.shiro.subject.support.SubjectThreadState; +import org.apache.shiro.util.LifecycleUtils; +import org.apache.shiro.util.ThreadState; +import org.junit.AfterClass; + +/** + * Abstract test case enabling Shiro in test environments. + * Copy&Paste from https://shiro.apache.org/testing.html + */ +public abstract class AbstractShiroTest { + + private static ThreadState subjectThreadState; + + public AbstractShiroTest() { + } + + /** + * Allows subclasses to set the currently executing {@link Subject} instance. + * + * @param subject the Subject instance + */ + protected void setSubject(Subject subject) { + clearSubject(); + subjectThreadState = createThreadState(subject); + subjectThreadState.bind(); + } + + protected Subject getSubject() { + return SecurityUtils.getSubject(); + } + + protected ThreadState createThreadState(Subject subject) { + return new SubjectThreadState(subject); + } + + /** + * Clears Shiro's thread state, ensuring the thread remains clean for future test execution. + */ + protected void clearSubject() { + doClearSubject(); + } + + private static void doClearSubject() { + if (subjectThreadState != null) { + subjectThreadState.clear(); + subjectThreadState = null; + } + } + + protected static void setSecurityManager(SecurityManager securityManager) { + SecurityUtils.setSecurityManager(securityManager); + } + + protected static SecurityManager getSecurityManager() { + return SecurityUtils.getSecurityManager(); + } + + @AfterClass + public static void tearDownShiro() { + doClearSubject(); + try { + SecurityManager securityManager = getSecurityManager(); + LifecycleUtils.destroy(securityManager); + } catch (UnavailableSecurityManagerException e) { + // we don't care about this when cleaning up the test environment + // (for example, maybe the subclass is a unit test and it didn't + // need a SecurityManager instance because it was using only + // mock Subject instances) + } + setSecurityManager(null); + } +} Review Comment: nit: add a new line
          githubbot ASF GitHub Bot added a comment -

          jongyoul commented on code in PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1164412779

          ##########
          zeppelin-server/src/test/java/org/apache/zeppelin/service/shiro/AbstractShiroTest.java:
          ##########
          @@ -0,0 +1,94 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one or more
          + * contributor license agreements. See the NOTICE file distributed with
          + * this work for additional information regarding copyright ownership.
          + * The ASF licenses this file to You under the Apache License, Version 2.0
          + * (the "License"); you may not use this file except in compliance with
          + * the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +package org.apache.zeppelin.service.shiro;
          +
          +import org.apache.shiro.SecurityUtils;
          +import org.apache.shiro.UnavailableSecurityManagerException;
          +import org.apache.shiro.mgt.SecurityManager;
          +import org.apache.shiro.subject.Subject;
          +import org.apache.shiro.subject.support.SubjectThreadState;
          +import org.apache.shiro.util.LifecycleUtils;
          +import org.apache.shiro.util.ThreadState;
          +import org.junit.AfterClass;
          +
          +/**
          + * Abstract test case enabling Shiro in test environments.
          + * Copy&Paste from https://shiro.apache.org/testing.html
          + */
          +public abstract class AbstractShiroTest {
          +
          + private static ThreadState subjectThreadState;
          +
          + public AbstractShiroTest()

          { + }

          +
          + /**
          + * Allows subclasses to set the currently executing

          {@link Subject}

          instance.
          + *
          + * @param subject the Subject instance
          + */
          + protected void setSubject(Subject subject)

          { + clearSubject(); + subjectThreadState = createThreadState(subject); + subjectThreadState.bind(); + }

          +
          + protected Subject getSubject()

          { + return SecurityUtils.getSubject(); + }

          +
          + protected ThreadState createThreadState(Subject subject)

          { + return new SubjectThreadState(subject); + }

          +
          + /**
          + * Clears Shiro's thread state, ensuring the thread remains clean for future test execution.
          + */
          + protected void clearSubject()

          { + doClearSubject(); + }

          +
          + private static void doClearSubject() {
          + if (subjectThreadState != null)

          { + subjectThreadState.clear(); + subjectThreadState = null; + }

          + }
          +
          + protected static void setSecurityManager(SecurityManager securityManager)

          { + SecurityUtils.setSecurityManager(securityManager); + }

          +
          + protected static SecurityManager getSecurityManager()

          { + return SecurityUtils.getSecurityManager(); + }

          +
          + @AfterClass
          + public static void tearDownShiro() {
          + doClearSubject();
          + try

          { + SecurityManager securityManager = getSecurityManager(); + LifecycleUtils.destroy(securityManager); + }

          catch (UnavailableSecurityManagerException e)

          { + // we don't care about this when cleaning up the test environment + // (for example, maybe the subclass is a unit test and it didn't + // need a SecurityManager instance because it was using only + // mock Subject instances) + }

          + setSecurityManager(null);
          + }
          +}

          Review Comment:
          ```suggestion
          }

          ```

          githubbot ASF GitHub Bot added a comment - jongyoul commented on code in PR #4580: URL: https://github.com/apache/zeppelin/pull/4580#discussion_r1164412779 ########## zeppelin-server/src/test/java/org/apache/zeppelin/service/shiro/AbstractShiroTest.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.zeppelin.service.shiro; + +import org.apache.shiro.SecurityUtils; +import org.apache.shiro.UnavailableSecurityManagerException; +import org.apache.shiro.mgt.SecurityManager; +import org.apache.shiro.subject.Subject; +import org.apache.shiro.subject.support.SubjectThreadState; +import org.apache.shiro.util.LifecycleUtils; +import org.apache.shiro.util.ThreadState; +import org.junit.AfterClass; + +/** + * Abstract test case enabling Shiro in test environments. + * Copy&Paste from https://shiro.apache.org/testing.html + */ +public abstract class AbstractShiroTest { + + private static ThreadState subjectThreadState; + + public AbstractShiroTest() { + } + + /** + * Allows subclasses to set the currently executing {@link Subject} instance. + * + * @param subject the Subject instance + */ + protected void setSubject(Subject subject) { + clearSubject(); + subjectThreadState = createThreadState(subject); + subjectThreadState.bind(); + } + + protected Subject getSubject() { + return SecurityUtils.getSubject(); + } + + protected ThreadState createThreadState(Subject subject) { + return new SubjectThreadState(subject); + } + + /** + * Clears Shiro's thread state, ensuring the thread remains clean for future test execution. + */ + protected void clearSubject() { + doClearSubject(); + } + + private static void doClearSubject() { + if (subjectThreadState != null) { + subjectThreadState.clear(); + subjectThreadState = null; + } + } + + protected static void setSecurityManager(SecurityManager securityManager) { + SecurityUtils.setSecurityManager(securityManager); + } + + protected static SecurityManager getSecurityManager() { + return SecurityUtils.getSecurityManager(); + } + + @AfterClass + public static void tearDownShiro() { + doClearSubject(); + try { + SecurityManager securityManager = getSecurityManager(); + LifecycleUtils.destroy(securityManager); + } catch (UnavailableSecurityManagerException e) { + // we don't care about this when cleaning up the test environment + // (for example, maybe the subclass is a unit test and it didn't + // need a SecurityManager instance because it was using only + // mock Subject instances) + } + setSecurityManager(null); + } +} Review Comment: ```suggestion } ```
          githubbot ASF GitHub Bot added a comment -

          Reamer merged PR #4580:
          URL: https://github.com/apache/zeppelin/pull/4580

          githubbot ASF GitHub Bot added a comment - Reamer merged PR #4580: URL: https://github.com/apache/zeppelin/pull/4580
          Reamer Philipp Dallig added a comment -

          Powermock has been removed.

          Reamer Philipp Dallig added a comment - Powermock has been removed.

          People

            Reamer Philipp Dallig
            Reamer Philipp Dallig
            Votes:
            1 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack