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 2021/05/10 02:17:39 UTC

[GitHub] [accumulo] EdColeman opened a new pull request #2087: Fix #2070 address ShellServerIT test failures

EdColeman opened a new pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087


   - replaces https://github.com/apache/accumulo/pull/2073
   
   - address simple check-style errors
   - use unique table and namespace names
   - Add test for constraints jar built with 2_1
   - Address pr comments
     - removed auto-format changes
     - moved assignment to eliminate possible null warnings


-- 
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

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

Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629496618



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.

Review comment:
       OK I opened up a separate issue to fix the history test. https://github.com/apache/accumulo/issues/2091




-- 
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

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

Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629334957



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.
     ts.exec("history", true, table, true);
+    // TODO - what is this testing?

Review comment:
       This is ideally testing to see if the history command ran previously shows up in this history check. Which it does in the shell. Unsure if this is working correctly due to the issue mentioned above.




-- 
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

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

Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629334242



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.

Review comment:
       There does appear to be an issue with the history test. Changing the name of the table does result in a test failure. I am not quite sure if it has to do with the history command itself or with the way we search for the expectedString. 




-- 
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

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

Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629376844



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.

Review comment:
       This issue seems to occur in 1.10 as well. Interestingly, the @test annotation for this test was commented out in the 2.0.0 and 2.0.1 release and was uncommented in #1483.  




-- 
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

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

Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629512994



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.
     ts.exec("history", true, table, true);
+    // TODO - what is this testing?

Review comment:
       I believe it was just to test that 'history' showed up in the history. I do agree that it didn't make sense since the table was also called 'history'.




-- 
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

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

Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087#discussion_r629501934



##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1421,21 +1431,24 @@ public void help() throws Exception {
     }
   }
 
+  // TODO - evaluate this test is testing what is expected or history is working.
   @Test
   public void history() throws Exception {
     final String table = name.getMethodName();
 
     ts.exec("history -c", true);
     ts.exec("createtable " + table);
     ts.exec("deletetable -f " + table);
+    // TODO - this may be passing only because method name is history.
     ts.exec("history", true, table, true);
+    // TODO - what is this testing?

Review comment:
       What I was marking - testing for a table named "history" and a string "history" seems either redundant - or was trying to test something else.  It could be that the string test was to see that history was returned as part of the command and when something else besides history is the table name then it makes more sense.




-- 
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

[GitHub] [accumulo] milleruntime merged pull request #2087: Fix #2070 address ShellServerIT test failures

Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2087:
URL: https://github.com/apache/accumulo/pull/2087


   


-- 
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