-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
acceptance: Refactors java acceptance tests #10659
acceptance: Refactors java acceptance tests #10659
Conversation
9ad1058
to
b897fb6
Compare
CI failure looks related to some issues on master (should go away with a rebase). |
While you're rebasing, please include the entire PR description in your commit message. |
Fixes cockroachdb#4591 This moves the java acceptance test to separate java files inside `acceptance/java` package to ease editing. Positive java test cases live under `acceptance/java/success` and negative test cases under `acceptance/java/fail`. The go test code invokes a bash scripts 'run_java_test.sh' which compiles and runs 'TestRunner.java` inside the test docker container. 'TestRunner.java` is responsible for running all java test under 'acceptance/java/success` and 'acceptance/java/fail'. It does so by accepting 2 command line arguments [expectedOutcome] [test_dir]. 'TestRunner' also makes sure that all generated files (.class and .pk8) files are cleaned up.
@tamird Rebased with master and included the description in the commit message |
b897fb6
to
0a27601
Compare
Thanks for your help. Some minor comments left. Reviewed 3 of 6 files at r1. pkg/acceptance/java/TestRunner.java, line 31 at r1 (raw file):
In English there is no space between a colon (:) and the previous word. e.g. pkg/acceptance/java/TestRunner.java, line 37 at r1 (raw file):
The null value doesn't say the argument was missing, but rather that it wasn't correct. Make the exception more informative. pkg/acceptance/java/TestRunner.java, line 88 at r1 (raw file):
Simplify: also, use pkg/acceptance/java/TestRunner.java, line 179 at r1 (raw file):
Missing newline at end of file. pkg/acceptance/java/fail/JavaFailureTest.java, line 46 at r1 (raw file):
Move this test into a separate file. pkg/acceptance/java/fail/JavaFailureTest.java, line 67 at r1 (raw file):
Move this test (this CREATE together with the INSERT below) into a separate file. pkg/acceptance/java/success/JavaSuccessTest.java, line 48 at r1 (raw file):
Split test here (see comment above) pkg/acceptance/java/success/JavaSuccessTest.java, line 69 at r1 (raw file):
Split test here (see comment above) Comments from Reviewable |
Reviewed 4 of 6 files at r1. pkg/acceptance/java/run_cleanup.sh, line 4 at r1 (raw file):
pkg/acceptance/java/run_java_test.sh, line 2 at r1 (raw file):
pkg/acceptance/java/run_java_test.sh, line 9 at r1 (raw file):
how come this is necessary? pkg/acceptance/java/TestRunner.java, line 9 at r1 (raw file):
perhaps i'm missing something, but why is this test runner written in java? it seems to me that this thing amounts to little more than shelling out to various tools, and that there is little or no benefit to it being written in java. given that java is not a core competency for us, I would prefer to see this written in Go. pkg/acceptance/java/TestRunner.java, line 59 at r1 (raw file):
broken sentence. pkg/acceptance/java/fail/JavaFailureTest.java, line 7 at r1 (raw file):
this connection string construction code is duplicated; can it be shared? Comments from Reviewable |
Review status: 4 of 6 files reviewed at latest revision, 14 unresolved discussions, all commit checks successful. pkg/acceptance/java/TestRunner.java, line 9 at r1 (raw file):
Comments from Reviewable |
Fixes #4591
This moves the java acceptance test to separate java files inside
acceptance/java
package to ease editing. Positive java test cases live underacceptance/java/success
and negative test cases underacceptance/java/fail
. The go test code invokes a bash scriptsrun_java_test.sh
which compiles and runsTestRunner.java
inside the test docker container.TestRunner.java
is responsible for running all java test underacceptance/java/success
andacceptance/java/fail
. It does so by accepting 2 command line arguments[expectedOutcome]
[test_dir]
.TestRunner
also makes sure that all generated files (.class and .pk8) files are cleaned up.This change is