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/01/21 19:26:38 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
DomGarguilo opened a new pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427
It was suggested that we look into upgrading from JUnit 4 to JUnit 5. I wanted to take an initial stab at this migration. From what I have gathered so far this is the upgrade path:
1. Replace JUnit 4 dependencies with JUnit 5 and JUnit "vintage-engine". The vintage engine is a stand-in for the JUnit 4 dependency to enable JUnit 4 tests to run.
2. Convert each test from JUnit 4 to JUnit 5. This does not have to happen all at once and we could opt to not upgrade some tests for whatever reason if we wanted.
3. Once all tests are converted to JUnit 5, we can remove the JUnit "vintage-engine" dependency.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1018903080
For reference, I saw [this link](https://stackoverflow.com/questions/47158583/executing-junit-4-and-junit-5-tests-in-a-same-build) shows how to get both JUnit4 and JUnit5 tests to build in the same module. If we upgrade one module at a time, we can just switch that module's engine from vintage to jupiter. Also, there's a bunch of examples in their [git repo](https://github.com/junit-team/junit5-samples).
I think the migration via updating the POMs is probably simple enough, but the trickier part might be if some tests using the vintage engine don't work with PowerMock. Or, if we are stuck staying on the vintage engine because some uses of PowerMock might prevent us from using the Jupiter engine. The only way to know is to try and work through the test failures. Currently, tests are failing for other reasons, so it may be hard to migrate right now until those are resolved.
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1041919024
I've marked this PR as ready for review. I know that there is a lot to look through when reviewing so I thought it might be helpful to leave some notes to help reviewers know what they are looking at.
>
> Here are some quick notes that might help to reference for those who work on the migration or for reviewers.
>
> | JUnit4 | JUnit5 |
> | ----------- | ----------- |
> | `@Test` | `@Test` |
> | `@Before` | `@BeforeEach` |
> | `@After` | `@AfterEach` |
> | `@BeforeClass` | `@BeforeAll` |
> | `@AfterClass` | `AfterAll` |
> | `@Ignore` | `@Disable` |
> | `@Category` | `@Tag` |
>
> | | JUnit4 | JUnit5 |
> | ----------- | ----------- | ----------- |
> | Testing class package | `org.junit` | `org.junit.jupiter.api` |
> | Assertions class | `Assert` | `Assertions` |
>
> `TemporaryFolder` was removed and instead a `File` or `Path` is used in JUnit5 with the `@TempDir` annotation.
_Originally posted by @DomGarguilo in https://github.com/apache/accumulo/issues/2441#issuecomment-1041908425_
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r810687659
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
I had an alternate idea for this, in my latest code review, using a `Function<TestInfo,String>` or similar. The suggestion above could also be done that way, but having it live in AccumuloITBase would only affect integration tests. The tests in other modules would still benefit from using something shorter in many of the tests. See my recent code review for an example.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r790018859
##########
File path: pom.xml
##########
@@ -610,6 +605,16 @@
<artifactId>jline</artifactId>
<version>3.20.0</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <version>5.8.2</version>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <version>5.8.2</version>
+ </dependency>
Review comment:
It might be possible to manage all the junit dependencies with a single bom:
```suggestion
<dependency>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.8.2</version>
<type>pom</type>
<scope>import</scope>
</dependency>
```
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792685784
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -128,33 +129,33 @@ public void testGetInstanceID_Direct() {
assertEquals(IID_STRING, zki.getInstanceID());
}
- @Test(expected = RuntimeException.class)
+ @Test
public void testGetInstanceID_NoMapping() {
ClientConfiguration config = createMock(ClientConfiguration.class);
expect(zc.get(Constants.ZROOT + Constants.ZINSTANCES + "/instance")).andReturn(null);
replay(zc);
EasyMock.reset(config, zcf);
- new ZooKeeperInstance(config, zcf);
+ assertThrows(RuntimeException.class, () -> new ZooKeeperInstance(config, zcf));
Review comment:
Do you think it would be a good idea to make a separate PR and take care of all of these cases first? Might make sense since even if we decide to not upgrade to JUnit 5 for whatever reason, that could still be a working improvement for our JUnit 4 tests.
--
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
[GitHub] [accumulo] DomGarguilo edited a comment on pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1041919024
I've marked this PR as ready for review. I know that there is a lot to look through when reviewing so I thought it might be helpful to leave some notes to help reviewers know what they are looking at.
>
> Here are some quick notes that might help to reference for those who work on the migration or for reviewers.
>
> | JUnit4 | JUnit5 |
> | ----------- | ----------- |
> | `@Test` | `@Test` |
> | `@Before` | `@BeforeEach` |
> | `@After` | `@AfterEach` |
> | `@BeforeClass` | `@BeforeAll` |
> | `@AfterClass` | `AfterAll` |
> | `@Ignore` | `@Disable` |
> | `@Category` | `@Tag` |
>
> | | JUnit4 | JUnit5 |
> | ----------- | ----------- | ----------- |
> | Testing class package | `org.junit` | `org.junit.jupiter.api` |
> | Assertions class | `Assert` | `Assertions` |
>
> `TemporaryFolder` was removed and instead a `File` or `Path` is used in JUnit5 with the `@TempDir` annotation.
>
>Additionally, the order of parameters in assert statements has changed. The optional message is now the last parameter rather than the first parameter. e.g. `assertTrue("should be equal", 1==3)` becomes `assertTrue(1==3, "should be equal")`
_Originally posted by @DomGarguilo in https://github.com/apache/accumulo/issues/2441#issuecomment-1041908425_
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792955737
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -128,33 +129,33 @@ public void testGetInstanceID_Direct() {
assertEquals(IID_STRING, zki.getInstanceID());
}
- @Test(expected = RuntimeException.class)
+ @Test
public void testGetInstanceID_NoMapping() {
ClientConfiguration config = createMock(ClientConfiguration.class);
expect(zc.get(Constants.ZROOT + Constants.ZINSTANCES + "/instance")).andReturn(null);
replay(zc);
EasyMock.reset(config, zcf);
- new ZooKeeperInstance(config, zcf);
+ assertThrows(RuntimeException.class, () -> new ZooKeeperInstance(config, zcf));
Review comment:
Yes. These would be good to have first. Then, you can rebase this onto that, in order to make this migration smaller and easier to review.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r810895229
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
I found the best way to do it is to create a superclass for the test that has a `testName()` method and a `@BeforeEach` annotation that sets the test name from the `TestInfo` class. See `WithTestNames` in the core module. The same can be done for other modules that need it. However, the class would need to be duplicated for any modules that use it, because of the way test code does not have dependencies on test code in other modules, and because we can't put the test class with JUnit types into the main jar, or else it would add a JUnit dependency there, which we don't want.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1021639443
> I have one whole module converted now (accumulo-core) with all tests passing (that were passing before these changes). There are build failures due to declared-unused and unused-declared dependencies. I didn't run into any big issues yet so I'll move on to a new module now.
This is going to get *really* big. So, instead of adding all the changes to one single big PR, which is a nightmare for code review, let's split this across a couple different PRs, so we can make sure that each incremental update still works before we move on to the next one.
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r805016285
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -128,33 +129,33 @@ public void testGetInstanceID_Direct() {
assertEquals(IID_STRING, zki.getInstanceID());
}
- @Test(expected = RuntimeException.class)
+ @Test
public void testGetInstanceID_NoMapping() {
ClientConfiguration config = createMock(ClientConfiguration.class);
expect(zc.get(Constants.ZROOT + Constants.ZINSTANCES + "/instance")).andReturn(null);
replay(zc);
EasyMock.reset(config, zcf);
- new ZooKeeperInstance(config, zcf);
+ assertThrows(RuntimeException.class, () -> new ZooKeeperInstance(config, zcf));
Review comment:
addressed in #2435
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1021610143
I have one whole module converted now (accumulo-core) with all tests passing (that were passing before these changes). There are build failures due to declared-unused and unused-declared dependencies. I didn't run into any big issues yet so I'll move on to a new module now.
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: WIP - Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1040778857
I think this is pretty much finished aside from some dependency related issues. The build is failing from a "used-undeclared" error within `accumulo-start` module. I think this is happening because the tests that have not been converted to JUnit 5 tests should be running on the junit vintage-engine dependency however, since junit 4 is part of the `org.apache.hadoop:hadoop-client-minicluster:jar:3.3.0` (which is in the accumulo-start pom), it is using the junit 4 dependency instead and complaining that vintage-engine is **not used but declared** and junit 4 is **used but undeclared**. I'm not too sure how this can be handled aside from somehow ignoring the warnings for now and converting to junit 5.
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r808310490
##########
File path: pom.xml
##########
@@ -610,6 +605,16 @@
<artifactId>jline</artifactId>
<version>3.20.0</version>
</dependency>
+ <dependency>
+ <groupId>org.junit.jupiter</groupId>
+ <artifactId>junit-jupiter-api</artifactId>
+ <version>5.8.2</version>
+ </dependency>
+ <dependency>
+ <groupId>org.junit.vintage</groupId>
+ <artifactId>junit-vintage-engine</artifactId>
+ <version>5.8.2</version>
+ </dependency>
Review comment:
addressed in [466348b](https://github.com/apache/accumulo/pull/2427/commits/466348b9a2e23f776be47c54b236545d2b62805c)
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792137858
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -128,33 +129,33 @@ public void testGetInstanceID_Direct() {
assertEquals(IID_STRING, zki.getInstanceID());
}
- @Test(expected = RuntimeException.class)
+ @Test
public void testGetInstanceID_NoMapping() {
ClientConfiguration config = createMock(ClientConfiguration.class);
expect(zc.get(Constants.ZROOT + Constants.ZINSTANCES + "/instance")).andReturn(null);
replay(zc);
EasyMock.reset(config, zcf);
- new ZooKeeperInstance(config, zcf);
+ assertThrows(RuntimeException.class, () -> new ZooKeeperInstance(config, zcf));
Review comment:
FWIW, the current version of JUnit supports this. This is something that we can easily apply to all our code today, and it would make this migration go more smoothly, at least for the code review portion.
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
The display name can contain invalid characters (commas) that aren't suitable for table names. It's better to use the `.getTestMethod().get()` (or some other variant that ensures it's not null, like `.getTestMethod().orElseThrow(IllegalStateException::new)`).
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -22,7 +22,8 @@
import static org.easymock.EasyMock.createMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review comment:
It would be very useful if all of our uses of `assert*()` methods were already using static imports. If that were done already in our existing code, it would make this migration go more smoothly, because only the import statement would change, which makes reviewing easier. We may already be using static imports for these everywhere, though.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792959175
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -22,7 +22,8 @@
import static org.easymock.EasyMock.createMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review comment:
You are right. It looks like I added some checkstyle rules for that awhile back. These rules will need to be updated at some point before this migration is fully done:
https://github.com/apache/accumulo/blob/912292d2131dbaf0f9cc28aff464608b478bf362/pom.xml#L1084-L1091
--
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
[GitHub] [accumulo] ctubbsii edited a comment on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1022535919
> > This is going to get _really_ big. So, instead of adding all the changes to one single big PR, which is a nightmare for code review, let's split this across a couple different PRs, so we can make sure that each incremental update still works before we move on to the next one.
>
> That sounds like a good idea. Do you have anything in mind for how we might split this task up? Maybe Each module?
Tasks can be grouped into:
1. things that can be done while still on JUnit4 to ease the migration
2. things to update the parent POM to manage the JUnit dependencies
3. (module migration stage 1) things that can be done in each module to move that module from JUnit4 to JUnit5 with the vintage engine
4. (module migration stage 2) things that can be done in each module to move that module from JUnit5 vintage to JUnit5 jupiter engine
The first item can definitely be done in its own PR. The last two can be broken up a few different ways, depending. Like, you could do item 3 for all modules in one PR, then do separate PRs for each module (or a couple modules at a time, if they are small enough to be manageable for a review) for item 4. For item 2, you can probably include that in your first PR that does any JUnit5 stuff.
Feel free to break it up however you think best. The goal in breaking it up is to help streamline code reviews and make incremental progress without breaking anything.
You could keep a table to track the migration:
| Module | Migrate to JUnit5 vintage | Migrate to JUnit5 jupiter |
| ----------- | ----------- | ----------- |
| assemble | ☑ | ☑ |
| core | ☑ | ☑ |
| hadoop-mapreduce | ☑ | |
| iterator-test-harness | ☑ | |
| minicluster | ☑ | |
| server/base | ☑ | |
| server/compaction-coordinator | ☑ | |
| server/compactor | ☑ | |
| server/gc | ☑ | |
| server/manager | ☑ | |
| server/master | ☑ | |
| server/monitor | ☑ | |
| server/native | ☑ | |
| server/tserver | ☑ | |
| shell | ☑ | |
| start | ☑ | |
| test | | |
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1022540279
Great ideas, @ctubbsii! Thanks!
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1022535919
> > This is going to get _really_ big. So, instead of adding all the changes to one single big PR, which is a nightmare for code review, let's split this across a couple different PRs, so we can make sure that each incremental update still works before we move on to the next one.
>
> That sounds like a good idea. Do you have anything in mind for how we might split this task up? Maybe Each module?
Tasks can be grouped into:
1. things that can be done while still on JUnit4 to ease the migration
2. things to update the parent POM to manage the JUnit dependencies
3. (module migration stage 1) things that can be done in each module to move that module from JUnit4 to JUnit5 with the vintage engine
4. (module migration stage 2) things that can be done in each module to move that module from JUnit5 vintage to JUnit5 jupiter engine
The first item can definitely be done in its own PR. The last two can be broken up a few different ways, depending. Like, you could do item 3 for all modules in one PR, then do separate PRs for each module (or a couple modules at a time, if they are small enough to be manageable for a review) for item 4. For item 2, you can probably include that in your first PR that does any JUnit5 stuff.
Feel free to break it up however you think best. The goal in breaking it up is to help streamline code reviews and make incremental progress without breaking anything.
You could keep a table to track the migration:
| Syntax | Migrate to JUnit5 vintage | Migrate to JUnit5 jupiter |
| ----------- | ----------- | ----------- |
| assemble | ☑ | ☑ |
| core | ☑ | ☑ |
| hadoop-mapreduce | ☑ | |
| iterator-test-harness | ☑ | |
| minicluster | ☑ | |
| server/base | ☑ | |
| server/compaction-coordinator | ☑ | |
| server/compactor | ☑ | |
| server/gc | ☑ | |
| server/manager | ☑ | |
| server/master | ☑ | |
| server/monitor | ☑ | |
| server/native | ☑ | |
| server/tserver | ☑ | |
| shell | ☑ | |
| start | ☑ | |
| test | | |
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792965730
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
That's a bit bulky to replace everywhere. I'd put a static method somewhere. Maybe in `AccumuloITBase`:
```java
public static testMethodName(TestInfo testInfo) {
return testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
}
```
That way, you can do:
```java
import static org.apache.accumulo.harness.AccumuloITBase.testMethodName;
// ...
@Test
public void someTestName(TestInfo testInfo) {
String table1Name = testMethodName(testInfo) + "1";
// ...
}
```
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r805018157
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
I like this idea. I think I'll forgo creating that method until I work on that module (accumulo-test, where AccumuloITBase resides) but I have left a note about creating the method and using it in the TODO section on ticket #2441
--
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
[GitHub] [accumulo] DomGarguilo edited a comment on pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo edited a comment on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1047874646
@ctubbsii, thanks for the review and changes! Everything looks good to me.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1022544383
> Great ideas, @ctubbsii! Thanks!
Sure, no problem. Oh, and the table view is nice, because it helps visualize different ways you can incrementally make progress. You could do separate PRs to chunk things vertically (migrate to vintage for several modules at once), or horizontally (fully migrate one module), or some rectangular section (like fully migrate a few or all of the smallest modules in one PR), or you could do a separate PR for each separate cell in the table (might be useful for knocking out the biggest modules).
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792745084
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
Good point. I was looking for a good replacement for `.getMethodName()` and from some quick testing after your suggestions, it looks like `testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName()` might be a suitable replacement.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792965730
##########
File path: core/src/test/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormatTest.java
##########
@@ -29,24 +29,20 @@
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.getDisplayName() + "1";
Review comment:
That's a bit bulky to replace everywhere. I'd put a static method somewhere. Maybe in `AccumuloITBase`:
```java
public static String testMethodName(TestInfo testInfo) {
return testInfo.getTestMethod().orElseThrow(IllegalStateException::new).getName();
}
```
That way, you can do:
```java
import static org.apache.accumulo.harness.AccumuloITBase.testMethodName;
// ...
@Test
public void someTestName(TestInfo testInfo) {
String table1Name = testMethodName(testInfo) + "1";
// ...
}
```
--
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
[GitHub] [accumulo] ctubbsii merged pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r810688012
##########
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:
```suggestion
```
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r810887966
##########
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:
I figured it out. This was leftover from an earlier iteration of this where the display name was used instead of the test method name. I'll remove it. It's no longer needed, since we're not using the display name.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: WIP - Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1040786445
> I'm not too sure how this can be handled aside from somehow ignoring the warnings for now and converting to junit 5.
The warnings will cause the build to fail, so they should be suppressed. This can be done in the pom.xml section that configures the dependency analyzer. You can just leave a brief XML comment above the suppression explaining when it can be removed.
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1047874646
@ctubbsii, thanks for the review and suggestions! Everything looks good to me.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2427: Migrate accumulo-core module from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1046326215
All the ITs passed with the changes so far. I saw a few tests flake out a bit, because they were timing sensitive and the operation they were waiting on took slightly longer than expected, but they passed on subsequent runs. And, I think that happened because I had another build running on the same machine at the same time. I couldn't reproduce those failures.
If you don't get to my suggestions above first, I may just make them myself, and merge this in. I don't want to risk this PR diverging further from the current passing state, with all the ITs passing and risking my absurdly time-consuming code review I just did on these 250 or so files getting stale. :smiley_cat:
--
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
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#discussion_r792708042
##########
File path: core/src/test/java/org/apache/accumulo/core/client/ZooKeeperInstanceTest.java
##########
@@ -22,7 +22,8 @@
import static org.easymock.EasyMock.createMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
-import static org.junit.Assert.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
Review comment:
I think it may already be the case that all `assert*()` methods are static imports
--
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
[GitHub] [accumulo] DomGarguilo commented on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1022288828
> This is going to get _really_ big. So, instead of adding all the changes to one single big PR, which is a nightmare for code review, let's split this across a couple different PRs, so we can make sure that each incremental update still works before we move on to the next one.
That sounds like a good idea. Do you have anything in mind for how we might split this task up? Maybe Each module?
--
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
[GitHub] [accumulo] ctubbsii edited a comment on pull request #2427: WIP - Migrate from JUnit 4 to JUnit 5
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on pull request #2427:
URL: https://github.com/apache/accumulo/pull/2427#issuecomment-1018903080
For reference, I saw [this link](https://stackoverflow.com/questions/47158583/executing-junit-4-and-junit-5-tests-in-a-same-build) shows how to get both JUnit4 and JUnit5 tests to build in the same module. If we upgrade one module at a time, we can just switch that module's engine from vintage to jupiter instead of trying to run both in the same module. Also, there's a bunch of examples in their [git repo](https://github.com/junit-team/junit5-samples).
I think the migration via updating the POMs is probably simple enough, but the trickier part might be if some tests using the vintage engine don't work with PowerMock. Or, if we are stuck staying on the vintage engine because some uses of PowerMock might prevent us from using the Jupiter engine. The only way to know is to try and work through the test failures. Currently, tests are failing for other reasons, so it may be hard to migrate right now until those are resolved.
--
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