You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2021/02/24 22:33:01 UTC
[GitHub] [hadoop] jbrennan333 commented on a change in pull request #2722: MAPREDUCE-7320. organize test directories for ClusterMapReduceTestCase
jbrennan333 commented on a change in pull request #2722:
URL: https://github.com/apache/hadoop/pull/2722#discussion_r582272551
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
+ * created.
+ * @return the Path of the root directory.
+ */
+ public static Path setupTestRootDir(Class<?> testClass,
+ String defaultTargetRootDir) {
+ // setup the test root directory
+ String targetTestDir =
+ System.getProperty(SYSPROP_TEST_DATA_DIR, defaultTargetRootDir);
+ Path testRootDirPath =
+ new Path(targetTestDir, testClass.getSimpleName());
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
+ testRootDirPath.toString());
Review comment:
I'm a little uneasy about overwriting this property. I think I see why you are doing it, so getTestDir will do the right thing, but I worry that it might lead to unexpected behavior because getTestDir is used everywhere. At a minimum, I think the comment on the function should clearly state that this changes the property.
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -245,6 +270,18 @@ public static File getTestDir() {
return dir;
}
+ /**
+ * Cleans-up the root directory from the property
+ * {@link #SYSPROP_TEST_DATA_DIR}.
+ *
+ * @return the absolute file of the test root directory.
+ */
+ public static File clearTestRootDir() {
Review comment:
This is only called from setupTestRootDir(), so I don't think it needs to be a separate public function. I would just do the fullyDelete() call in-line in setup TestRootDir.
##########
File path: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
+ private static Path testRootDir;
+
private MiniDFSCluster dfsCluster = null;
- private MiniMRCluster mrCluster = null;
+ private MiniMRClientCluster mrCluster = null;
+
+ protected static void setupClassBase(Class<?> testClass) throws Exception {
+ // setup the test root directory
+ testRootDir = GenericTestUtils.setupTestRootDir(testClass,
+ TEST_ROOT_DEFAULT_PATH);
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
Review comment:
I don't think we should set the property here if we are doing it in setupTestRootDir().
##########
File path: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
+ private static Path testRootDir;
+
private MiniDFSCluster dfsCluster = null;
- private MiniMRCluster mrCluster = null;
+ private MiniMRClientCluster mrCluster = null;
+
+ protected static void setupClassBase(Class<?> testClass) throws Exception {
+ // setup the test root directory
+ testRootDir = GenericTestUtils.setupTestRootDir(testClass,
+ TEST_ROOT_DEFAULT_PATH);
+ System.setProperty(GenericTestUtils.SYSPROP_TEST_DATA_DIR,
Review comment:
One question is whether we should get the original value and then reset it in an afterClass method? Is every unit test run in a separate jvm instance? Wouldn't want a following test to get the wrong setting.
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
Review comment:
This description is a little unclear. Maybe default relative path to use if SYSPROP_TEST_DATA_DIR is not set?
##########
File path: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/ClusterMapReduceTestCase.java
##########
@@ -43,8 +45,21 @@
* The DFS filesystem is formated before the testcase starts and after it ends.
*/
public abstract class ClusterMapReduceTestCase {
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
Review comment:
This seems redundant, since GenericTestUtils.setupTestRootDir() does the same thing. Can't we just use GenericTestUtils.DEFAULT_TEST_DATA_DIR? Note that GenericTestUtils.DEFAULT_TEST_DATA_DIR has the advantage of being correct in both linux and windows, where the hard-coded strings will only work in linux.
I'm actually not sure you need to pass a default to GenericTestUtils.setupTestRootDir.
##########
File path: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/security/ssl/TestEncryptedShuffle.java
##########
@@ -31,37 +30,34 @@
import org.apache.hadoop.mapreduce.MRConfig;
import org.apache.hadoop.security.ssl.KeyStoreTestUtil;
+import org.apache.hadoop.test.GenericTestUtils;
import org.apache.hadoop.util.StringUtils;
import org.apache.hadoop.yarn.conf.YarnConfiguration;
import org.junit.After;
-import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.Assert;
-import java.io.BufferedReader;
import java.io.File;
-import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.Writer;
-import java.net.URL;
public class TestEncryptedShuffle {
- private static final String BASEDIR =
- System.getProperty("test.build.dir", "target/test-dir") + "/" +
- TestEncryptedShuffle.class.getSimpleName();
-
+ private static final String TEST_ROOT_DEFAULT_PATH =
+ System.getProperty("test.build.data", "target/test-dir");
Review comment:
Similar to above - just use DEFAULT_TEST_DATA_DIR instead of this. Or remove it if we decide we don't need the default for setupTestRootDir().
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
Review comment:
I'm actually not sure we need to pass a default here. Everywhere we use it, it is just using DEFAULT_TEST_DATA_DIR.
##########
File path: hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/test/GenericTestUtils.java
##########
@@ -229,6 +229,31 @@ public static int uniqueSequenceId() {
return sequence.incrementAndGet();
}
+ /**
+ * Creates a directory for the data/logs of the unit test.
+ * It first delete the directory if it exists.
+ *
+ * @param testClass the unit test class.
+ * @param defaultTargetRootDir the directory where the class directory is
+ * created.
+ * @return the Path of the root directory.
+ */
+ public static Path setupTestRootDir(Class<?> testClass,
+ String defaultTargetRootDir) {
+ // setup the test root directory
+ String targetTestDir =
+ System.getProperty(SYSPROP_TEST_DATA_DIR, defaultTargetRootDir);
Review comment:
Maybe just use DEFAULT_TEST_DATA_DIR for the default value. Might also want to add the check for prop.isEmpty() like in getTestDir().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org