You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/02/19 15:50:42 UTC
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r810497366
##########
File path: core/src/test/java/org/apache/accumulo/fate/util/RetryTest.java
##########
@@ -96,8 +96,8 @@ public void usingNonExistentRetryFails() {
retry.useRetry();
}
assertFalse(retry.canRetry());
- assertThrows("Calling useRetry when canRetry returns false throws an exception",
- IllegalStateException.class, () -> retry.useRetry());
+ assertThrows(IllegalStateException.class, () -> retry.useRetry(),
+ "Calling useRetry when canRetry " + "returns false throws an exception");
Review comment:
```suggestion
"Calling useRetry when canRetry returns false throws an exception");
```
##########
File path: iterator-test-harness/pom.xml
##########
@@ -47,6 +42,11 @@
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-client-api</artifactId>
</dependency>
+ <!--TODO Don't force downstream users to have JUnit -->
Review comment:
Can just remove this comment. There's no problem having JUnit as a dependency.
##########
File path: core/src/test/java/org/apache/accumulo/fate/util/RetryTest.java
##########
@@ -276,8 +276,8 @@ public void testMaxWait() {
builder.maxWait(15, MILLISECONDS);
builder.maxWait(16, MILLISECONDS);
- assertThrows("Max wait time should be greater than or equal to initial wait time",
- IllegalArgumentException.class, () -> builder.maxWait(14, MILLISECONDS));
+ assertThrows(IllegalArgumentException.class, () -> builder.maxWait(14, MILLISECONDS),
+ "Max wait time " + "should be greater than or equal to initial wait time");
Review comment:
```suggestion
"Max wait time should be greater than or equal to initial wait time");
```
##########
File path: core/src/test/java/org/apache/accumulo/core/security/CredentialsTest.java
##########
@@ -35,20 +36,17 @@
import org.apache.accumulo.core.clientImpl.Credentials;
import org.apache.accumulo.core.data.InstanceId;
import org.apache.accumulo.core.securityImpl.thrift.TCredentials;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
public class CredentialsTest {
- @Rule
- public TestName test = new TestName();
-
private InstanceId instanceID;
Review comment:
Could get rid of this member variable and replace it with a function, to make it easier to create the local variables in each test:
(don't need to `orElseThrow`, because it will already throw `NoSuchElementException`)
```suggestion
private Function<TestInfo, InstanceId> testInfoToInstanceId = testInfo -> InstanceId.of(testInfo.getTestMethod().get().getName());
```
##########
File path: core/src/test/java/org/apache/accumulo/core/file/rfile/bcfile/CompressionTest.java
##########
@@ -267,16 +271,16 @@ public void testThereCanBeOnlyOne() throws InterruptedException, ExecutionExcept
results.addAll(service.invokeAll(list));
// ensure that we
- assertEquals(al + " created too many codecs", 1, testSet.size());
+ assertEquals(1, testSet.size(), al + " created too many codecs");
service.shutdown();
while (!service.awaitTermination(1, TimeUnit.SECONDS)) {
// wait
}
for (Future<Boolean> result : results) {
- assertTrue(al + " resulted in a failed call to getcodec within the thread pool",
- result.get());
+ assertTrue(result.get(),
+ al + " resulted in a failed call to getcodec within the thread " + "pool");
Review comment:
```suggestion
al + " resulted in a failed call to getcodec within the thread pool");
```
##########
File path: core/src/test/java/org/apache/accumulo/core/security/CredentialsTest.java
##########
@@ -35,20 +36,17 @@
import org.apache.accumulo.core.clientImpl.Credentials;
import org.apache.accumulo.core.data.InstanceId;
import org.apache.accumulo.core.securityImpl.thrift.TCredentials;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
public class CredentialsTest {
- @Rule
- public TestName test = new TestName();
-
private InstanceId instanceID;
@Test
- public void testToThrift() throws DestroyFailedException {
- instanceID = InstanceId.of(test.getMethodName());
+ public void testToThrift(TestInfo testInfo) throws DestroyFailedException {
+ instanceID =
+ InstanceId.of(testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName());
Review comment:
To use the aforementioned Function inside each of these methods:
```suggestion
var instanceID = testInfoToInstanceId.apply(testInfo);
```
##########
File path: core/src/test/java/org/apache/accumulo/core/data/TableIdTest.java
##########
@@ -61,7 +58,8 @@ public void testCacheNoDuplicates() {
assertNotSame(RootTable.ID, MetadataTable.ID);
assertNotSame(RootTable.ID, REPL_TABLE_ID);
- String tableString = "table-" + name.getMethodName();
+ String tableString =
+ "table-" + testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
Review comment:
```suggestion
String tableString = "table-" + testInfo.getTestMethod().get().getName();
```
##########
File path: core/src/test/java/org/apache/accumulo/core/data/TableIdTest.java
##########
@@ -83,15 +81,17 @@ public void testCacheNoDuplicates() {
assertSame(table1, table2);
}
- @Test(timeout = 30_000)
- public void testCacheIncreasesAndDecreasesAfterGC() {
+ @Test
+ @Timeout(30_000)
+ public void testCacheIncreasesAndDecreasesAfterGC(TestInfo testInfo) {
long initialSize = cacheCount();
assertTrue(initialSize < 20); // verify initial amount is reasonably low
LOG.info("Initial cache size: {}", initialSize);
LOG.info(TableId.cache.asMap().toString());
// add one and check increase
- String tableString = "table-" + name.getMethodName();
+ String tableString =
+ "table-" + testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
Review comment:
```suggestion
String tableString = "table-" + testInfo.getTestMethod().get().getName();
```
##########
File path: core/src/test/java/org/apache/accumulo/core/spi/balancer/HostRegexTableLoadBalancerTest.java
##########
@@ -79,9 +79,10 @@ public void init(Map<String,String> tableProperties) {
@Test
public void testInit() {
init(DEFAULT_TABLE_PROPERTIES);
- assertEquals("OOB check interval value is incorrect", 7000, this.getOobCheckMillis());
- assertEquals("Max migrations is incorrect", 4, this.getMaxMigrations());
- assertEquals("Max outstanding migrations is incorrect", 10, this.getMaxOutstandingMigrations());
+ assertEquals(7000, this.getOobCheckMillis(), "OOB check interval value is incorrect");
+ assertEquals(4, this.getMaxMigrations(), "Max migrations is incorrect");
+ assertEquals(10, this.getMaxOutstandingMigrations(),
+ "Max outstanding migrations is " + "incorrect");
Review comment:
```suggestion
assertEquals(10, this.getMaxOutstandingMigrations(), "Max outstanding migrations is incorrect");
```
##########
File path: core/src/test/java/org/apache/accumulo/core/data/NamespaceIdTest.java
##########
@@ -71,15 +69,17 @@ public void testCacheNoDuplicates() {
assertSame(nsId, nsId2);
}
- @Test(timeout = 30_000)
- public void testCacheIncreasesAndDecreasesAfterGC() {
+ @Test
+ @Timeout(30_000)
+ public void testCacheIncreasesAndDecreasesAfterGC(TestInfo testInfo) {
long initialSize = cacheCount();
assertTrue(initialSize < 20); // verify initial amount is reasonably low
LOG.info("Initial cache size: {}", initialSize);
LOG.info(NamespaceId.cache.asMap().toString());
// add one and check increase
- String namespaceString = "namespace-" + name.getMethodName();
+ String namespaceString =
+ "namespace-" + testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
Review comment:
```suggestion
String namespaceString = "namespace-" + testInfo.getTestMethod().get().getName();
```
##########
File path: core/src/test/java/org/apache/accumulo/core/data/NamespaceIdTest.java
##########
@@ -37,22 +37,20 @@
private static final Logger LOG = LoggerFactory.getLogger(NamespaceIdTest.class);
- @Rule
- public TestName name = new TestName();
-
private static long cacheCount() {
// guava cache size() is approximate, and can include garbage-collected entries
// so we iterate to get the actual cache size
return NamespaceId.cache.asMap().entrySet().stream().count();
}
@Test
- public void testCacheNoDuplicates() {
+ public void testCacheNoDuplicates(TestInfo testInfo) {
// the next line just preloads the built-ins, since they now exist in a separate class from
// NamespaceId, and aren't preloaded when the NamespaceId class is referenced
assertNotSame(Namespace.ACCUMULO.id(), Namespace.DEFAULT.id());
- String namespaceString = "namespace-" + name.getMethodName();
+ String namespaceString =
+ "namespace-" + testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
Review comment:
```suggestion
String namespaceString = "namespace-" + testInfo.getTestMethod().get().getName();
```
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapreduce/AccumuloMultiTableInputFormatTest.java
##########
@@ -30,23 +30,21 @@
import org.apache.accumulo.core.util.Pair;
import org.apache.hadoop.io.Text;
import org.apache.hadoop.mapreduce.Job;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
@Deprecated(since = "2.0.0")
public class AccumuloMultiTableInputFormatTest {
- @Rule
- public TestName testName = new TestName();
-
/**
* Verify {@link InputTableConfig} objects get correctly serialized in the JobContext.
*/
@Test
- public void testInputTableConfigSerialization() throws IOException {
- String table1 = testName.getMethodName() + "1";
- String table2 = testName.getMethodName() + "2";
+ public void testInputTableConfigSerialization(TestInfo testInfo) throws IOException {
+ String table1 =
+ testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName() + "1";
+ String table2 =
+ testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName() + "2";
Review comment:
```suggestion
String table1 = testInfo.getTestMethod().get().getName() + "1";
String table2 = testInfo.getTestMethod().get().getName() + "2";
```
##########
File path: core/src/test/java/org/apache/accumulo/core/conf/DeprecatedPropertyUtilTest.java
##########
@@ -89,9 +89,9 @@ public void testSanityCheckManagerProperties() {
DeprecatedPropertyUtil.sanityCheckManagerProperties(config); // should succeed
config.setProperty("manager.replacementProp", "value");
assertEquals(4, config.size());
- assertThrows("Sanity check should fail when 'master.*' and 'manager.*' appear in same config",
- IllegalStateException.class,
- () -> DeprecatedPropertyUtil.sanityCheckManagerProperties(config));
+ assertThrows(IllegalStateException.class,
+ () -> DeprecatedPropertyUtil.sanityCheckManagerProperties(config),
+ "Sanity check should " + "fail when 'master.*' and 'manager.*' appear in same config");
Review comment:
```suggestion
"Sanity check should fail when 'master.*' and 'manager.*' appear in same config");
```
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,22 @@
import org.apache.accumulo.core.util.Pair;
import org.apache.hadoop.io.Text;
import org.apache.hadoop.mapred.JobConf;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
@Deprecated(since = "2.0.0")
public class AccumuloMultiTableInputFormatTest {
- @Rule
- public TestName testName = new TestName();
-
/**
* Verify {@link org.apache.accumulo.core.client.mapreduce.InputTableConfig} objects get correctly
* serialized in the JobContext.
*/
@Test
- public void testTableQueryConfigSerialization() {
- String table1Name = testName.getMethodName() + "1";
- String table2Name = testName.getMethodName() + "2";
+ public void testTableQueryConfigSerialization(TestInfo testInfo) {
+ String table1Name =
+ testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName() + "1";
+ String table2Name =
+ testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName() + "2";
Review comment:
```suggestion
String table1Name = testInfo.getTestMethod().get().getName() + "1";
String table2Name = testInfo.getTestMethod().get().getName() + "2";
```
##########
File path: core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java
##########
@@ -18,38 +18,39 @@
*/
package org.apache.accumulo.core.conf;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayNameGeneration;
+import org.junit.jupiter.api.DisplayNameGenerator;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
import com.google.common.base.Joiner;
+@DisplayNameGeneration(DisplayNameGenerator.Simple.class)
public class PropertyTypeTest {
- @Rule
- public TestName testName = new TestName();
- private PropertyType type = null;
+ private PropertyType type;
- @Before
- public void getPropertyTypeForTest() {
- String tn = testName.getMethodName();
- if (tn.startsWith("testType")) {
+ @BeforeEach
+ public void getPropertyTypeForTest(TestInfo testInfo) {
+ String displayName = testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
Review comment:
```suggestion
String displayName = testInfo.getTestMethod().get().getName();
```
##########
File path: core/src/test/java/org/apache/accumulo/core/file/BloomFilterLayerLookupTest.java
##########
@@ -84,7 +80,9 @@ public void test() throws IOException {
// get output file name
String suffix = FileOperations.getNewFileExtension(acuconf);
- String fname = new File(tempDir.getRoot(), testName + "." + suffix).getAbsolutePath();
+ String fname = new File(tempDir,
+ testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName() + "." + suffix)
Review comment:
Already throws NoSuchElementException on .get if empty:
```suggestion
String fname = new File(tempDir, testInfo.getTestMethod().get().getName() + "." + suffix)
```
##########
File path: core/src/test/java/org/apache/accumulo/core/conf/PropertyTypeTest.java
##########
@@ -18,38 +18,39 @@
*/
package org.apache.accumulo.core.conf;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TestName;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayNameGeneration;
+import org.junit.jupiter.api.DisplayNameGenerator;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
import com.google.common.base.Joiner;
+@DisplayNameGeneration(DisplayNameGenerator.Simple.class)
Review comment:
Do we need to use this display name generator? What's wrong with the default?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org