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/12 14:15:15 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #2097: Fix history test inside ShellServerIT
Manno15 opened a new pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097
Fixes #2091.
The history command was failing due to the executed commands not being written to the history file. This change will write the expected commands to the history file but solely for the history test at this moment. Some other tests break when this change was applied to every 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631098158
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
ts.exec("history -c", true);
Review comment:
There may be an opportunity here to validate that the clean command worked as expected.
--
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 merged pull request #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631220510
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
+
+ // test that history command prints contents of history file
ts.exec("history", true, table, true);
- // TODO - what is this testing?
- ts.exec("history", true, "history", true);
Review comment:
In a previous iteration, I kept this line but replaced the expected string with "foo". That is now being used in the validity check so I removed it here. The rest of what you said is still true and with your additions above, I believe the history command is thoroughly tested.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631218588
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
+
+ // test that history command prints contents of history file
ts.exec("history", true, table, true);
Review comment:
I agree on this point, I should have done that earlier.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631109776
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
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.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
Review comment:
Does the write to history perform an exec() - previously the test was executing a command, testing the command was successful (via the exec status) and then that appeared in the history - from a quick look I don't see where this performs the exec. Not that these commands need tested, but is it a valid test of shell 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] Manno15 commented on a change in pull request #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631127302
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
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.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
Review comment:
These aren't executed. I felt it was safe to leave it out from an exec call since they aren't part of this test specifically. The exec command fails to write to the history file due how things are set up with the shell. The history file is part of the reader, but the reader isn't involved with the executions since I assume it doesn't interpret it as an input command (Only for this IT, things work normally inside the actual shell). This is why only `ts.input` would show up in the history file since it was being read.
From a validity standpoint, the exec of the history command does work properly and the contents of the reader's history file gets displayed.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631127302
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
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.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
Review comment:
These aren't executed. I felt it was safe to leave it out from an exec call since they aren't part of this test specifically. The exec command fails to write to the history file due how things are set up with the shell. The history file is part of the reader, but the reader isn't involved with the executions since I assume it doesn't interpret it as an input command. This is why only `ts.input` would show up in the history file since it was being read.
From a validity standpoint, the exec of the history command does work properly and the contents of the reader's history file gets displayed.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631101636
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
ts.exec("history -c", true);
Review comment:
Good idea.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631215488
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
Review comment:
Fair point, I was assuming the string present would account for the clear command as well and do the check after clearing. Unsure if that is the case but it was failing when I had the stringPresent set to true
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631147765
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,15 @@ 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();
+ final String table = getUniqueNames(1)[0];
ts.exec("history -c", true);
Review comment:
I added a short validity check for the clear command in my latest commit
--
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] ctubbsii commented on a change in pull request #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631233866
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
Review comment:
I'm pretty sure `history -c` just clears, and doesn't print anything. So, a check for any string present at all would have been false.
--
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 #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631218263
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
Review comment:
If validity check for the clear works correctly, then I am not sure this step is necessary but I don't have anything against being thorough so I will add.
--
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] ctubbsii commented on a change in pull request #2097: Fix history test inside ShellServerIT
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2097:
URL: https://github.com/apache/accumulo/pull/2097#discussion_r631208892
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
Review comment:
This could verify that the table name and these commands are not present first:
```suggestion
// write to history file. Does not execute functions
ts.exec("history", true, table, false);
ts.exec("history", true, "createtable", false);
ts.exec("history", true, "deletetable", false);
ts.writeToHistory("createtable " + table);
```
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
Review comment:
This didn't first verify that "foo" was present, or that it was removed afterwards:
```suggestion
ts.exec("history", true, "foo", true);
ts.exec("history -c", true);
ts.exec("history", true, "foo", false);
```
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
+
+ // test that history command prints contents of history file
ts.exec("history", true, table, true);
- // TODO - what is this testing?
- ts.exec("history", true, "history", true);
Review comment:
I think this was just a basic test ensuring that exec caused the command to be added to the history. If that doesn't work in the test case, because of the way the test is written, that's fine to remove this, so long as executing commands in the shell still appends to the history.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -1431,18 +1436,20 @@ 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();
+ final String table = getUniqueNames(1)[0];
- 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.
+ // test clear history command works
+ ts.writeToHistory("foo");
+ ts.exec("history -c", true, "foo", false);
+
+ // write to history file. Does not execute functions
+ ts.writeToHistory("createtable " + table);
+ ts.writeToHistory("deletetable -f " + table);
+
+ // test that history command prints contents of history file
ts.exec("history", true, table, true);
Review comment:
This could be more thorough in checking that the createtable and deletetable commands were added:
```suggestion
ts.exec("history", true, "createtable " + table, true);
ts.exec("history", true, "deletetable -f " + table, true);
```
--
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