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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
