Details
-
Sub-task
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
-
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
Attachments
Issue Links
Activity
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 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.
- What is this PR for?
-
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?
-
-
-
- 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
Reamer commented on PR #4519:
URL: https://github.com/apache/zeppelin/pull/4519#issuecomment-1404800082
I need a review.
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
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?
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
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?
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
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.
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
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.
jongyoul merged PR #4519:
URL: https://github.com/apache/zeppelin/pull/4519
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 is this PR for?
-
-
-
- What type of PR is it?
-
- Refactoring
-
-
- What is the Jira issue?
-
-
-
- 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
Reamer commented on PR #4580:
URL: https://github.com/apache/zeppelin/pull/4580#issuecomment-1501121841
I need a review.
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
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.
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
instance.
+ *
+ * @param subject the Subject instance
+ */
+ protected void setSubject(Subject subject)
+
+ protected Subject getSubject()
+
+ protected ThreadState createThreadState(Subject subject)
+
+ /**
+ * Clears Shiro's thread state, ensuring the thread remains clean for future test execution.
+ */
+ protected void clearSubject()
+
+ private static void doClearSubject() {
+ if (subjectThreadState != null)
+ }
+
+ protected static void setSecurityManager(SecurityManager securityManager)
+
+ protected static SecurityManager getSecurityManager()
+
+ @AfterClass
+ public static void tearDownShiro() {
+ doClearSubject();
+ try
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
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
instance.
+ *
+ * @param subject the Subject instance
+ */
+ protected void setSubject(Subject subject)
+
+ protected Subject getSubject()
+
+ protected ThreadState createThreadState(Subject subject)
+
+ /**
+ * Clears Shiro's thread state, ensuring the thread remains clean for future test execution.
+ */
+ protected void clearSubject()
+
+ private static void doClearSubject() {
+ if (subjectThreadState != null)
+ }
+
+ protected static void setSecurityManager(SecurityManager securityManager)
+
+ protected static SecurityManager getSecurityManager()
+
+ @AfterClass
+ public static void tearDownShiro() {
+ doClearSubject();
+ try
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
}
```
Reamer merged PR #4580:
URL: https://github.com/apache/zeppelin/pull/4580
Hi Philipp Dallig,
I think this ticket is very neccesary. I'm interested in it.
Do you have started to deal with this ticket?