Refactor the Java acceptance test.

[knz]

This patch pulls out Java code in separate standalone .java files
and generalizes template substitution.

[bdarnell]

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

acceptance/java_test.go, line 30 [r1] (raw file):
The substitution is pretty ugly; I think we should get rid of it so the tests are plain non-pre-processed java files. The point of testDockerFail is to make sure that if the test failed the response code would be passed back correctly, so we just need some way to force an error at runtime (a command-line flag or environment variable)

---

acceptance/main.java, line 1 [r1] (raw file):
It will get pretty messy to have files in lots of different languages in the acceptance directory; can we make a subdirectory acceptance/java?

---

acceptance/main.java, line 6 [r1] (raw file):
Use either tabs or spaces for indentation; don't mix them and assume that a tab is equivalent to 8 spaces. I think we've standardized on 2-space indents for non-go code.

---

acceptance/main.java, line 20 [r1] (raw file):
How does this even work? We're not applying the substitution to main.java

---

Comments from the review on Reviewable.io

[tamird]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/main.java, line 1 [r1] (raw file):
+1

---

acceptance/TestStatements.tmpl.java, line 7 [r1] (raw file):
man, this really does not seem worth the interpolation now. Can we just write plain java and have two tests?

---

Comments from the review on Reviewable.io

[maddyblue]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/main.java, line 6 [r1] (raw file):
We didn't standardize on using tabs for tabs just like go does? :(

---

Comments from the review on Reviewable.io

[maddyblue]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/java_test.go, line 30 [r1] (raw file):
I think the reason it's being done this way is that a future PR is adding a few more java tests, and would like to use the common connection logic.

---

Comments from the review on Reviewable.io

[tamird]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/java_test.go, line 30 [r1] (raw file):
What if we compiled a single JAR that ran all our java tests?

---

Comments from the review on Reviewable.io

[knz]

Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/java_test.go, line 30 [r1] (raw file):
Yes please see how I am using this in https://github.com/knz/cockroach/blob/empty-row-desc/acceptance/java_test.go.

---

acceptance/main.java, line 1 [r1] (raw file):
ok I'll have a look.

---

acceptance/main.java, line 6 [r1] (raw file):
Ow 😭

---

acceptance/main.java, line 20 [r1] (raw file):
Oops wrong file committed.

---

acceptance/TestStatements.tmpl.java, line 7 [r1] (raw file):
Yeah I think that might be best.

---

Comments from the review on Reviewable.io

[bdarnell]

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

acceptance/java_test.go, line 30 [r1] (raw file):
Preprocessing the source is not the way to go. The tests should be customizable using the command line or environment so the source doesn't have to change. We should generally just have one testDockerSuccess call per language which will run all of that language's tests (which should be defined in the language's standard idioms wherever possible - is junit or some equivalent part of the standard library for java these days or would we have to figure out how to handle external java dependencies to be able to use it?)

---

acceptance/main.java, line 6 [r1] (raw file):
I'd be fine with tabs-only as well, but we went with 2-space indents for our .ts files.

---

Comments from the review on Reviewable.io

[knz]

Ok guys this is not the ultimate Java acceptance refactoring PR. I really want to get something going in order to move forward with #4548 and #4550 and ultimately #4036. I propose that once this PR is merged and I can go on with #4548, we file another issue to make this Java testing even better than it is now post-Beta.

[knz]

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.

---

acceptance/java_test.go, line 30 [r1] (raw file):
@bdarnell yes this is possible however there is no preprocessing going on any more in the latest commit.

---

acceptance/main.java, line 6 [r1] (raw file):
Spaces it is.

---

Comments from the review on Reviewable.io

[tamird]

LGTM

---

Reviewed 8 of 10 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

acceptance/java/runjava.sh, line 1 [r3] (raw file):
nit: no space after the shebang, and i think this can be sh

---

acceptance/java/runjava.sh, line 2 [r3] (raw file):
probably a good idea to set -u as well.

---

acceptance/java/runjava.sh, line 11 [r3] (raw file):
don't you always want this file compiled? i think you can drop this argument.

---

acceptance/java/runjava.sh, line 12 [r3] (raw file):
you should trap EXIT and clean this up, then you can avoid the -f

---

acceptance/java/TestPlaceholders.java, line 25 [r3] (raw file):
let's throw a better exception here, yeah?

---

Comments from the review on Reviewable.io

[knz]

Review status: 5 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

acceptance/java/runjava.sh, line 1 [r3] (raw file):
Done.

---

acceptance/java/runjava.sh, line 2 [r3] (raw file):
Done.

---

acceptance/java/runjava.sh, line 11 [r3] (raw file):
The test for #4550 will use a different DB URL.

---

acceptance/java/runjava.sh, line 12 [r3] (raw file):
Code saved vs code gained, etc.

---

acceptance/java/TestPlaceholders.java, line 25 [r3] (raw file):
Done.

---

Comments from the review on Reviewable.io

[bdarnell]

LGTM but I still think it would be worthwhile to make the test configurable at runtime instead of swapping out which classes get compiled in.

---

Review status: 3 of 6 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 1 of 10 files at r2, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

acceptance/java/runjava.sh, line 1 [r3] (raw file):
eh, i meant #!/usr/bin/env sh

---

acceptance/java/runjava.sh, line 2 [r3] (raw file):
sorry to nit, but we usually keep it alphabetical with set -eux

---

acceptance/java/runjava.sh, line 12 [r3] (raw file):
it's not about gaining or saving code, it's about not polluting the file system

---

acceptance/java/TestPlaceholders.java, line 25 [r3] (raw file):
Can we include the actual values? how is anyone going to debug this with the current message?

---

Comments from the review on Reviewable.io

[knz]

Review status: 5 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

acceptance/java/runjava.sh, line 1 [r3] (raw file):
That is not necessary. POSIX says 'sh' lives in /bin.

---

acceptance/java/runjava.sh, line 12 [r3] (raw file):
Right now this doesn't work anyways so I'll refactor.

---

acceptance/java/TestPlaceholders.java, line 25 [r3] (raw file):
This is a refactoring PR not enhancement. I'll file a separate issue.

---

Comments from the review on Reviewable.io

[knz]

Closing in favor of #4591.