[GitHub] [accumulo] ctubbsii commented on a change in pull request #2073: Fix #2070 address ShellServerIT test failures

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2073:
URL: https://github.com/apache/accumulo/pull/2073#discussion_r624870598



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2900,9 +2931,7 @@ private void generateSplitsFile(final String splitsFile, final int numItems, fin
   }
 
   private Text getRandomText(final int len) {
-    int desiredLen = len;
-    if (len > 32)
-      desiredLen = 32;
+    var desiredLen = Math.min(len, 32);

Review comment:
       This isn't a great use of `var`. `int` would be better here, since the type isn't obvious from the variable name or its assignment.

##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1717,7 +1719,7 @@ public void testPertableClasspath() throws Exception {
     fooFilterJar.deleteOnExit();
 
     File fooConstraintJar = File.createTempFile("FooConstraint", ".jar", new File(rootPath));
-    FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint.jar"),
+    FileUtils.copyInputStreamToFile(this.getClass().getResourceAsStream("/FooConstraint_2_1.jar"),

Review comment:
       I assume this new jar has constraints using the new public API package? I'm not sure it matters for this test. We should support older constraints as well, so this test should pass whether or not the jar contains classes that implement the new or old constraint interface.

##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -2687,13 +2705,15 @@ public void testCreateTableWithBinarySplitsFile1()
     try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) {
       splitsFile = System.getProperty("user.dir") + "/target/splitFile";
       generateSplitsFile(splitsFile, 200, 12, true, true, true, false, false);
-      SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile, false);
+      SortedSet<Text> expectedSplits = readSplitsFromFile(splitsFile);
       final String tableName = name.getMethodName() + "_table";
       ts.exec("createtable " + tableName + " -sf " + splitsFile, true);
       Collection<Text> createdSplits = client.tableOperations().listSplits(tableName);
       assertEquals(expectedSplits, new TreeSet<>(createdSplits));
     } finally {
-      Files.delete(Paths.get(splitsFile));
+      if (Objects.nonNull(splitsFile)) {

Review comment:
       `Objects.nonNull` exists for use as a lambda when a Predicate is required (`stream.filter(Objects::nonNull)`, for example). For plain old if statements, it's much more clear to just write:
   
   ```suggestion
         if (splitsFile != null) {
   ```
   
   However, there's no reason for it to ever be null or for us to check for it being null. We can just assign it to `System.getProperty("user.dir") + "/target/splitFile";` in the first place. There's no reason that needs to be done inside the try block.

##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -513,7 +515,7 @@ public void du() throws Exception {
     String o = ts.output.get();
     // for some reason, there's a bit of fluctuation
     assertTrue("Output did not match regex: '" + o + "'",
-        o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "\\]\\n"));
+        o.matches(".*[1-9][0-9][0-9]\\s\\[" + table + "]\\n"));

Review comment:
       Was this an unnecessary escape?

##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -340,16 +341,17 @@ public void setupShell() throws Exception {
   }
 
   @AfterClass
-  public static void tearDownAfterClass() {
+  public static void tearDownAfterClass() throws Exception {
     if (traceProcess != null) {
       traceProcess.destroy();
     }
 
+    deleteTables();
+
     SharedMiniClusterBase.stopMiniCluster();
   }
 
-  @After
-  public void deleteTables() throws Exception {
+  public static void deleteTables() throws Exception {

Review comment:
       I'm not sure it's worth bothering deleting tables at this point if we're killing mini anyway.
   
   I think this change would only matter if we were using the `parallel` options in `maven-failsafe-plugin`, which we aren't, and shouldn't because that would likely cause all sorts of other problems. The test methods that are run inside the class are always run serially. This delete tables method merely cleans up between different test methods in case they use the same table names or makes the system use too many resources. I don't think your change here accomplishes anything, except leaving tables around longer, possibly causing some methods to collide if they use the same table names.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org