build: allow directories for acceptance tests

[justinj]

🔬🐶, this commit lets acceptance tests specify a directory which should
be mounted.

Included is a test of jdbc which translates the old string literal test
into one which has a directory structure and can be opened in IntelliJ.

@jordanlewis could you also check that you can open the java_test directory in IntelliJ and have it load properly and stuff (you still have to use make acceptance TESTS=TestDockerJava to run it)?

[cockroach-teamcity]

This change is 

[benesch]

🎉 this is such an improvement. I didn't review the actual Java, FYI.

---

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

pkg/acceptance/java_test.go, line 31 at r1 (raw file):

	ctx := context.Background()
	classPath := ".:lib/junit.jar:lib/hamcrest.jar:lib/postgres.jar:src/main/java"

(I mentioned this below, but I'd be a very happy camper if these were instead baked into the postgres-test image at the appropriate classpath.)

---

pkg/acceptance/java_test.go, line 42 at r1 (raw file):

		cmd:   []string{"/bin/sh", "-c", strings.Replace(java, "%v", ``, 2)},
		binds: []string{"java_test"},
	})

I recommend dropping this test; every Docker test is so slow + flaky that we want each one to count.

---

pkg/acceptance/util_docker.go, line 51 at r1 (raw file):

	// name within the docker container as outside of it (see java_test.go which
	// mounts the java_test directory).
	binds []string

I would drop this from the struct, and just always bind testdata. Binds are cheap provided you don't read/write them.

---

pkg/acceptance/util_docker.go, line 99 at r1 (raw file):

		pwd, err = os.Getwd()
		if err != nil {
			return

eek! return err

also why predeclare pwd?

---

pkg/acceptance/java_test/.gitignore, line 5 at r1 (raw file):

.DS_Store
key.pk8
.idea

On principle, we don't store os- or editor-specific entries in our .gitignores. (See one of the other .gitignores for rationale.) That means .DS_Store and .idea should go, but *.class and target should stay. Is key.pk8 necessary to ignore, or just part of an auto-generated .gitignore?

---

pkg/acceptance/java_test/lib/hamcrest.jar, line 0 at r1 (raw file):
Ahhh, I'd really prefer not to check more binary blobs into the repo. I know it's a pain, but could you bake these into the postgres-test image?

---

pkg/acceptance/java_test/src/main/java/main.java, line 1 at r1 (raw file):

import org.junit.*;

Convention is to put files like this in a folder named testdata so that Go doesn't think it's a package if it contains .go files, etc. I'd advocate for testdata/java, and then always mount testdata.

---

Comments from Reviewable

[justinj]

Just responded to a couple things with clarification/questions - still working on the others

---

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

---

pkg/acceptance/java_test.go, line 42 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I recommend dropping this test; every Docker test is so slow + flaky that we want each one to count.

We actually have a bunch of tests like this that mess with their input to intentionally fail (I think Tamir told me it's because there was a time when they were failing but we weren't detecting said failures appropriately). Perhaps we could even get away with deleting all but one of them at this point? I think every acceptance test has two versions in this way.

---

pkg/acceptance/util_docker.go, line 51 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I would drop this from the struct, and just always bind testdata. Binds are cheap provided you don't read/write them.

👍 good point, I can just get rid of this struct and go back to the old function signatures then.

---

pkg/acceptance/util_docker.go, line 99 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

eek! return err

also why predeclare pwd?

The structure of this function is odd, the entire function predeclares err and returns it at the end due to the anonymous function passed to RunDocker (there's no explicit return at the end because the function ends there). Perhaps there's a nicer way to structure it given this change though, I'll investigate.

---

pkg/acceptance/java_test/.gitignore, line 5 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

On principle, we don't store os- or editor-specific entries in our .gitignores. (See one of the other .gitignores for rationale.) That means .DS_Store and .idea should go, but *.class and target should stay. Is key.pk8 necessary to ignore, or just part of an auto-generated .gitignore?

key.pk8 is generated by the test, it could conceivably delete it afterwards but then you'd have it sitting around if the test failed somehow I think, I think it's easier if it's just ignored.

I'll remove the other stuff, I sort of feel like .idea is a different case since you might not want use your main editor for Java stuff (and just use IntelliJ and then get out), but I don't feel strongly.

---

pkg/acceptance/java_test/lib/hamcrest.jar, line at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Ahhh, I'd really prefer not to check more binary blobs into the repo. I know it's a pain, but could you bake these into the postgres-test image?

It's nicer to work with them in an IDE if the IDE has access to them for autocomplete and stuff (which is why I put them in here), but I agree that their inclusion is unfortunate, I wonder if there's a way to get IntelliJ to download them and put them in an ignored directory... I'll look into that.

Would it be [overly] weird to .gitignore them and then
a. have the docker container place them in there when it runs or
b. include a script that can be run manually to download them if you want the IDE support?

---

Comments from Reviewable

[benesch]

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

---

pkg/acceptance/java_test.go, line 42 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

We actually have a bunch of tests like this that mess with their input to intentionally fail (I think Tamir told me it's because there was a time when they were failing but we weren't detecting said failures appropriately). Perhaps we could even get away with deleting all but one of them at this point? I think every acceptance test has two versions in this way.

Heh, you're totally right. I'm not sold on this being useful, but no point in breaking with convention.

---

pkg/acceptance/util_docker.go, line 51 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

👍 good point, I can just get rid of this struct and go back to the old function signatures then.

👍

---

pkg/acceptance/util_docker.go, line 99 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

The structure of this function is odd, the entire function predeclares err and returns it at the end due to the anonymous function passed to RunDocker (there's no explicit return at the end because the function ends there). Perhaps there's a nicer way to structure it given this change though, I'll investigate.

Oh, wow, I entirely misread. Carry on. Seems fine enough.

---

pkg/acceptance/java_test/.gitignore, line 5 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

key.pk8 is generated by the test, it could conceivably delete it afterwards but then you'd have it sitting around if the test failed somehow I think, I think it's easier if it's just ignored.

I'll remove the other stuff, I sort of feel like .idea is a different case since you might not want use your main editor for Java stuff (and just use IntelliJ and then get out), but I don't feel strongly.

Yeah, I know what you mean, but I have a moderately-strong preference for forcing people to put crap like that into their global gitignore. This has served me so well:

$ cat ~/.config/git/ignore
.DS_Store
.vscode
.idea
.terraform

(E.g., what if someone else working on Java prefers Eclipse or NetBeans or whatever?)

---

pkg/acceptance/java_test/lib/hamcrest.jar, line at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

It's nicer to work with them in an IDE if the IDE has access to them for autocomplete and stuff (which is why I put them in here), but I agree that their inclusion is unfortunate, I wonder if there's a way to get IntelliJ to download them and put them in an ignored directory... I'll look into that.

Would it be [overly] weird to .gitignore them and then
a. have the docker container place them in there when it runs or
b. include a script that can be run manually to download them if you want the IDE support?

Yeah, option b sounds perfect. I'd ignore all JAR files or lib entirely. Can you check in whatever mvn install needs, and then direct people to run mvn install? Does IntelliJ integrate with Maven?

---

Comments from Reviewable

[jordanlewis]

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

---

pkg/acceptance/java_test/.gitignore, line 5 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, I know what you mean, but I have a moderately-strong preference for forcing people to put crap like that into their global gitignore. This has served me so well:

$ cat ~/.config/git/ignore
.DS_Store
.vscode
.idea
.terraform

(E.g., what if someone else working on Java prefers Eclipse or NetBeans or whatever?)

-1 on including any intellij stuff in here at all. just use pom.xml. intellij (and every java ide) can import those

---

Comments from Reviewable

[tbg]

+1 on the approach, looking good!

---

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[justinj]

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

pkg/acceptance/java_test.go, line 31 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

(I mentioned this below, but I'd be a very happy camper if these were instead baked into the postgres-test image at the appropriate classpath.)

cockroachdb/postgres-test#25

---

pkg/acceptance/util_docker.go, line 51 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

👍

Done.

---

pkg/acceptance/java_test/lib/hamcrest.jar, line at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Yeah, option b sounds perfect. I'd ignore all JAR files or lib entirely. Can you check in whatever mvn install needs, and then direct people to run mvn install? Does IntelliJ integrate with Maven?

Ok, mvn install should work now! I added a README instructing the reader to do that.

---

pkg/acceptance/java_test/.gitignore, line 5 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

-1 on including any intellij stuff in here at all. just use pom.xml. intellij (and every java ide) can import those

Done.

---

Comments from Reviewable

[jordanlewis]

I'm pretty convinced that you should just make mvn test work. You're so close! Then you can get rid of any kind of weird javac invocations.

---

Review status: 0 of 8 files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.

---

pkg/acceptance/java_test.go, line 44 at r2 (raw file):

export PATH=$PATH:/usr/lib/jvm/java-1.8-openjdk/bin
javac -cp %v src/main/java/main.java
java -cp %v org.junit.runner.JUnitCore main

yay. fwiw this is kind of weird - normally you'd just run mvn test and have that all hooked up. I think this is fine - but just as a possibility if things get more complicated we can do that.

---

pkg/acceptance/testdata/java/java.iml, line 2 at r2 (raw file):

<?xml version="1.0" encoding="UTF-8"?>
<module org.jetbrains.idea.maven.project.MavenProjectsManager.isMavenModule="true" type="JAVA_MODULE" version="4">

this is an intellj file right? kill it with 🔥 :)

---

pkg/acceptance/testdata/java/pom.xml, line 25 at r2 (raw file):

      </dependency>
    </dependencies>
</project>

sweet! like i said above you can set this up so that mvn test actually runs all the JUnit tests. It might be nice...!

---

Comments from Reviewable

[tamird]

Does this close #4591?

[justinj]

It does, thanks for the heads up @tamird.

@jordanlewis good call, I didn't realize we would be able to do that - it uses mvn test now! Corresponding changes to the docker image here.

[benesch]

---

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

---

pkg/acceptance/java_test.go, line 41 at r3 (raw file):

openssl pkcs8 -topk8 -inform PEM -outform DER -in /certs/node.key -out key.pk8 -nocrypt

mvn %v

👍 this is short and sweet. Nice!

---

Comments from Reviewable

[benesch]

I think you just need to tag and push your postgres-test image and update util_docker.go accordingly in this PR?

[tbg]

BTW, re: the negative tests -- I think it's good not to have rot there, but it's not worth the CI time in the "hot path". I don't think I would trust a separate "sentinel test", but we could just run the negatives nightly-only and would find out pretty quickly if something did regress.

[justinj]

Thanks for the reviews!