You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@uniffle.apache.org by xi...@apache.org on 2023/02/08 05:36:46 UTC
[incubator-uniffle] branch master updated: [#559]test: use withEnvironmentVariables to replace setEnv in RSSUtils (#560)
This is an automated email from the ASF dual-hosted git repository.
xianjin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-uniffle.git
The following commit(s) were added to refs/heads/master by this push:
new b45175ef [#559]test: use withEnvironmentVariables to replace setEnv in RSSUtils (#560)
b45175ef is described below
commit b45175eff6b39c171b08ecea7fd96b537e4d92e0
Author: advancedxy <xi...@apache.org>
AuthorDate: Wed Feb 8 13:36:39 2023 +0800
[#559]test: use withEnvironmentVariables to replace setEnv in RSSUtils (#560)
### What changes were proposed in this pull request?
replace `RssUtils.setEnv` by `withEnvironmentVariable`
### Why are the changes needed?
Better code quality. This fixes #559
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existed UTs.
---
.../apache/uniffle/common/util/RssUtilsTest.java | 44 ++++++--------------
.../apache/uniffle/test/CoordinatorGrpcTest.java | 15 +++----
.../LocalSingleStorageTypeFromEnvProviderTest.java | 47 +++++++++-------------
.../uniffle/server/ShuffleServerConfTest.java | 25 +++++-------
.../server/storage/LocalStorageManagerTest.java | 19 ++++-----
5 files changed, 57 insertions(+), 93 deletions(-)
diff --git a/common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java b/common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java
index d054337f..4d5e0ca4 100644
--- a/common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java
+++ b/common/src/test/java/org/apache/uniffle/common/util/RssUtilsTest.java
@@ -17,7 +17,6 @@
package org.apache.uniffle.common.util;
-import java.lang.reflect.Field;
import java.net.Inet4Address;
import java.net.InetAddress;
import java.net.NetworkInterface;
@@ -42,6 +41,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariable;
public class RssUtilsTest {
@@ -66,18 +66,18 @@ public class RssUtilsTest {
assertFalse(ia.isLinkLocalAddress() || ia.isAnyLocalAddress() || ia.isLoopbackAddress());
assertNotNull(NetworkInterface.getByInetAddress(ia));
assertTrue(ia.isReachable(5000));
- setEnv("RSS_IP", "8.8.8.8");
- assertEquals("8.8.8.8", RssUtils.getHostIp());
- setEnv("RSS_IP", "xxxx");
- boolean isException = false;
- try {
- RssUtils.getHostIp();
- } catch (Exception e) {
- isException = true;
- }
- setEnv("RSS_IP", realIp);
- RssUtils.getHostIp();
- assertTrue(isException);
+ withEnvironmentVariable("RSS_IP", "8.8.8.8")
+ .execute(() -> assertEquals("8.8.8.8", RssUtils.getHostIp()));
+ withEnvironmentVariable("RSS_IP", "xxxx").execute(() -> {
+ boolean isException = false;
+ try {
+ RssUtils.getHostIp();
+ } catch (Exception e) {
+ isException = true;
+ }
+ assertTrue(isException);
+ });
+ withEnvironmentVariable("RSS_IP", realIp).execute(RssUtils::getHostIp);
} catch (Exception e) {
fail(e.getMessage());
}
@@ -201,24 +201,6 @@ public class RssUtilsTest {
}
}
- @SuppressWarnings("unchecked")
- public static void setEnv(String key, String value) {
- try {
- Map<String, String> env = System.getenv();
- Class<?> cl = env.getClass();
- Field field = cl.getDeclaredField("m");
- field.setAccessible(true);
- Map<String, String> writableEnv = (Map<String, String>) field.get(env);
- if (value != null) {
- writableEnv.put(key, value);
- } else {
- writableEnv.remove(key);
- }
- } catch (Exception e) {
- throw new IllegalStateException("Failed to set environment variable", e);
- }
- }
-
public static class RssUtilTestDummySuccess implements RssUtilTestDummy {
private final String s;
diff --git a/integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java b/integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java
index ce170a2b..c9abf04d 100644
--- a/integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java
+++ b/integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorGrpcTest.java
@@ -40,7 +40,6 @@ import org.apache.uniffle.common.storage.StorageInfo;
import org.apache.uniffle.common.storage.StorageMedia;
import org.apache.uniffle.common.storage.StorageStatus;
import org.apache.uniffle.common.util.Constants;
-import org.apache.uniffle.common.util.RssUtilsTest;
import org.apache.uniffle.coordinator.CoordinatorConf;
import org.apache.uniffle.coordinator.ServerNode;
import org.apache.uniffle.coordinator.SimpleClusterManager;
@@ -57,6 +56,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariables;
public class CoordinatorGrpcTest extends CoordinatorTestBase {
@@ -247,12 +247,13 @@ public class CoordinatorGrpcTest extends CoordinatorTestBase {
shuffleServerConf.set(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY, "RSS_ENV_KEY");
String baseDir = shuffleServerConf.get(ShuffleServerConf.RSS_STORAGE_BASE_PATH).get(0);
String storageTypeJsonSource = String.format("{\"%s\": \"ssd\"}", baseDir);
- RssUtilsTest.setEnv("RSS_ENV_KEY", storageTypeJsonSource);
- // set this server's tag to ssd
- shuffleServerConf.set(ShuffleServerConf.TAGS, Lists.newArrayList("SSD"));
- ShuffleServer ss = new ShuffleServer(shuffleServerConf);
- ss.start();
- shuffleServers.set(0, ss);
+ withEnvironmentVariables("RSS_ENV_KEY", storageTypeJsonSource).execute(() -> {
+ // set this server's tag to ssd
+ shuffleServerConf.set(ShuffleServerConf.TAGS, Lists.newArrayList("SSD"));
+ ShuffleServer ss = new ShuffleServer(shuffleServerConf);
+ ss.start();
+ shuffleServers.set(0, ss);
+ });
Thread.sleep(3000);
assertEquals(2, coordinators.get(0).getClusterManager().getNodesNum());
nodes = scm.getServerList(Sets.newHashSet(Constants.SHUFFLE_SERVER_VERSION, "SSD"));
diff --git a/server/src/test/java/org/apache/uniffle/server/LocalSingleStorageTypeFromEnvProviderTest.java b/server/src/test/java/org/apache/uniffle/server/LocalSingleStorageTypeFromEnvProviderTest.java
index f12ad45b..bc9bfac9 100644
--- a/server/src/test/java/org/apache/uniffle/server/LocalSingleStorageTypeFromEnvProviderTest.java
+++ b/server/src/test/java/org/apache/uniffle/server/LocalSingleStorageTypeFromEnvProviderTest.java
@@ -22,35 +22,22 @@ import java.util.Map;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.Maps;
-import org.junit.jupiter.api.AfterAll;
-import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.apache.uniffle.common.storage.StorageMedia;
-import org.apache.uniffle.common.util.RssUtilsTest;
import org.apache.uniffle.server.storage.StorageMediaFromEnvProvider;
import org.apache.uniffle.storage.common.StorageMediaProvider;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariables;
public class LocalSingleStorageTypeFromEnvProviderTest {
- private static String STORAGE_TYPE_ENV_KEY = "RSS_LOCAL_STORAGE_TYPES";
- private static String defaultStorageTypeSource;
+ private static final String STORAGE_TYPE_ENV_KEY = "RSS_LOCAL_STORAGE_TYPES";
private ShuffleServerConf rssConf;
private StorageMediaProvider provider;
- @BeforeAll
- static void setup() {
- defaultStorageTypeSource = System.getenv(STORAGE_TYPE_ENV_KEY);
- }
-
- @AfterAll
- static void reset() {
- RssUtilsTest.setEnv(STORAGE_TYPE_ENV_KEY, defaultStorageTypeSource);
- }
-
@BeforeEach
void setupRssServerConfig() {
rssConf = new ShuffleServerConf();
@@ -59,25 +46,28 @@ public class LocalSingleStorageTypeFromEnvProviderTest {
}
@Test
- public void testJsonSourceParse() {
+ public void testJsonSourceParse() throws Exception {
String emptyJsonSource = "";
- RssUtilsTest.setEnv(STORAGE_TYPE_ENV_KEY, emptyJsonSource);
- // invalid json source should not throw exceptions
- provider.init(rssConf);
- assertEquals(StorageMedia.UNKNOWN, provider.getStorageMediaFor("/data01"));
+ withEnvironmentVariables(STORAGE_TYPE_ENV_KEY, emptyJsonSource).execute(() -> {
+ // invalid json source should not throw exceptions
+ provider.init(rssConf);
+ assertEquals(StorageMedia.UNKNOWN, provider.getStorageMediaFor("/data01"));
+ });
emptyJsonSource = "{}";
- RssUtilsTest.setEnv(STORAGE_TYPE_ENV_KEY, emptyJsonSource);
- provider.init(rssConf);
- assertEquals(StorageMedia.UNKNOWN, provider.getStorageMediaFor("/data01"));
+ withEnvironmentVariables(STORAGE_TYPE_ENV_KEY, emptyJsonSource).execute(() -> {
+ provider.init(rssConf);
+ assertEquals(StorageMedia.UNKNOWN, provider.getStorageMediaFor("/data01"));
+ });
String storageTypeJson = "{\"/data01\": \"SSD\"}";
- RssUtilsTest.setEnv(STORAGE_TYPE_ENV_KEY, storageTypeJson);
- provider.init(rssConf);
- assertEquals(StorageMedia.SSD, provider.getStorageMediaFor("/data01"));
+ withEnvironmentVariables(STORAGE_TYPE_ENV_KEY, storageTypeJson).execute(() -> {
+ provider.init(rssConf);
+ assertEquals(StorageMedia.SSD, provider.getStorageMediaFor("/data01"));
+ });
}
@Test
- public void testMultipleMountPoints() {
+ public void testMultipleMountPoints() throws Exception {
Map<String, String> storageTypes = Maps.newHashMap();
storageTypes.put("/data01", "ssd");
storageTypes.put("/data02", "hdd");
@@ -89,9 +79,8 @@ public class LocalSingleStorageTypeFromEnvProviderTest {
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
- RssUtilsTest.setEnv(STORAGE_TYPE_ENV_KEY, jsonSource);
+ withEnvironmentVariables(STORAGE_TYPE_ENV_KEY, jsonSource).execute(() -> provider.init(rssConf));
- provider.init(rssConf);
assertEquals(StorageMedia.HDD, provider.getStorageMediaFor("/data02"));
assertEquals(StorageMedia.SSD, provider.getStorageMediaFor("/data01"));
assertEquals(StorageMedia.SSD, provider.getStorageMediaFor("/data03"));
diff --git a/server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java b/server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java
index faff7752..f70ca342 100644
--- a/server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java
+++ b/server/src/test/java/org/apache/uniffle/server/ShuffleServerConfTest.java
@@ -20,22 +20,16 @@ package org.apache.uniffle.server;
import java.io.File;
import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ExtendWith;
-import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
-import uk.org.webcompere.systemstubs.jupiter.SystemStub;
-import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;
import org.apache.uniffle.common.util.ByteUnit;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariables;
-@ExtendWith(SystemStubsExtension.class)
public class ShuffleServerConfTest {
private static final String confFile = ClassLoader.getSystemResource("confTest.conf").getFile();
- @SystemStub
- private static EnvironmentVariables environmentVariables;
@Test
public void defaultConfTest() {
@@ -47,14 +41,15 @@ public class ShuffleServerConfTest {
}
@Test
- public void envConfTest() {
- environmentVariables.set("RSS_HOME", (new File(confFile)).getParent());
- ShuffleServerConf shuffleServerConf = new ShuffleServerConf(null);
- assertEquals(1234, shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT));
- assertEquals("HDFS", shuffleServerConf.getString(ShuffleServerConf.RSS_STORAGE_TYPE));
- assertEquals("/var/tmp/test", shuffleServerConf.get(ShuffleServerConf.RSS_STORAGE_BASE_PATH).get(0));
- environmentVariables.set("RSS_HOME", (new File(confFile)).getParent() + "/wrong_dir/");
- assertFalse(shuffleServerConf.loadConfFromFile(null));
+ public void envConfTest() throws Exception {
+ withEnvironmentVariables("RSS_HOME", (new File(confFile)).getParent()).execute(() -> {
+ ShuffleServerConf shuffleServerConf = new ShuffleServerConf(null);
+ assertEquals(1234, shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT));
+ assertEquals("HDFS", shuffleServerConf.getString(ShuffleServerConf.RSS_STORAGE_TYPE));
+ assertEquals("/var/tmp/test", shuffleServerConf.get(ShuffleServerConf.RSS_STORAGE_BASE_PATH).get(0));
+ withEnvironmentVariables("RSS_HOME", (new File(confFile)).getParent() + "/wrong_dir/")
+ .execute(() -> assertFalse(shuffleServerConf.loadConfFromFile(null)));
+ });
}
@Test
diff --git a/server/src/test/java/org/apache/uniffle/server/storage/LocalStorageManagerTest.java b/server/src/test/java/org/apache/uniffle/server/storage/LocalStorageManagerTest.java
index b7a79760..6fdcf20e 100644
--- a/server/src/test/java/org/apache/uniffle/server/storage/LocalStorageManagerTest.java
+++ b/server/src/test/java/org/apache/uniffle/server/storage/LocalStorageManagerTest.java
@@ -33,7 +33,6 @@ import org.apache.uniffle.common.ShufflePartitionedBlock;
import org.apache.uniffle.common.storage.StorageInfo;
import org.apache.uniffle.common.storage.StorageMedia;
import org.apache.uniffle.common.storage.StorageStatus;
-import org.apache.uniffle.common.util.RssUtilsTest;
import org.apache.uniffle.server.ShuffleDataFlushEvent;
import org.apache.uniffle.server.ShuffleDataReadEvent;
import org.apache.uniffle.server.ShuffleServerConf;
@@ -48,6 +47,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.fail;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;
+import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariables;
/**
* The class is to test the {@link LocalStorageManager}
@@ -177,7 +177,7 @@ public class LocalStorageManagerTest {
}
@Test
- public void testInitializeLocalStorage() throws IOException {
+ public void testInitializeLocalStorage() {
// case1: when no candidates, it should throw exception.
ShuffleServerConf conf = new ShuffleServerConf();
@@ -247,7 +247,7 @@ public class LocalStorageManagerTest {
}
@Test
- public void testEnvStorageTypeProvider() {
+ public void testEnvStorageTypeProvider() throws Exception {
String[] storagePaths = {"/tmp/rss-data1"};
ShuffleServerConf conf = new ShuffleServerConf();
@@ -255,17 +255,14 @@ public class LocalStorageManagerTest {
conf.setLong(ShuffleServerConf.DISK_CAPACITY, 1024L);
conf.setString(ShuffleServerConf.RSS_STORAGE_TYPE, org.apache.uniffle.storage.util.StorageType.LOCALFILE.name());
conf.set(ShuffleServerConf.STORAGE_MEDIA_PROVIDER_ENV_KEY, "env_key");
- RssUtilsTest.setEnv("env_key", "{\"/tmp\": \"ssd\"}");
- LocalStorageManager localStorageManager = new LocalStorageManager(conf);
- Map<String, StorageInfo> storageInfo = localStorageManager.getStorageInfo();
- assertEquals(1, storageInfo.size());
- try {
+ withEnvironmentVariables("env_key", "{\"/tmp\": \"ssd\"}").execute(() -> {
+ LocalStorageManager localStorageManager = new LocalStorageManager(conf);
+ Map<String, StorageInfo> storageInfo = localStorageManager.getStorageInfo();
+ assertEquals(1, storageInfo.size());
String mountPoint = Files.getFileStore(new File("/tmp").toPath()).name();
assertNotNull(storageInfo.get(mountPoint));
// by default, it should report HDD as local storage type
assertEquals(StorageMedia.SSD, storageInfo.get(mountPoint).getType());
- } catch (IOException e) {
- throw new RuntimeException(e);
- }
+ });
}
}