Upgrade App Engine and webserver tests from JUnit 4 to 5

[CydeWeys]

This change is 

[CydeWeys]

Also, as a helpful suggestion for the reviewers, you'll want to focus most of your attention on the files that do not match the naming pattern _____Test.java. The changes in files that do match that naming pattern are pretty routine.

[hstonec]

Reviewed 6 of 237 files at r1.
Reviewable status: 6 of 237 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/server/RegistryTestServerMain.java, line 19 at r1 (raw file):

import com.beust.jcommander.JCommander;
import com.beust.jcommander.Parameter;
import com.beust.jcommander.Parameters;

I assume you tested this by running the server on your host?

---

core/src/test/java/google/registry/webdriver/RepeatableRunner.java, line 0 at r1 (raw file):
It seems that you deleted RepeatableRunner but not provided a JUnit5 replacement for its functionality to run the screenshot test multiple times until it succeeds. Based on our previous experience, if we only run the screenshot test once, sometimes the generated screenshot can be different from the golden image file. The internal service we used to depend on also provides this functionality.

---

core/src/test/java/google/registry/testing/AppEngineExtension.java, line 388 at r1 (raw file):

File queueFile = createFile(new File(tmpDir, "queue.xml").toPath()).toFile();

Why not just do File queueFile = new File(tmpDir, "queue.xml")?

[hstonec]

Reviewable status: 6 of 237 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/webdriver/RepeatableRunner.java, line at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

It seems that you deleted RepeatableRunner but not provided a JUnit5 replacement for its functionality to run the screenshot test multiple times until it succeeds. Based on our previous experience, if we only run the screenshot test once, sometimes the generated screenshot can be different from the golden image file. The internal service we used to depend on also provides this functionality.

I just noticed that you added @RetryingTest(3) for each of the test method, I guess that does the retrying.

[CydeWeys]

Reviewable status: 6 of 237 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/webdriver/RepeatableRunner.java, line at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I just noticed that you added @RetryingTest(3) for each of the test method, I guess that does the retrying.

Yes. This seemed to be the best way to accomplish the retrying, since runners aren't as much of a JUnit 5 thing and all the solutions I found out there were annotations on individual methods, not an extension or annotation that would work on a whole test class. And so long as all of the tests in a given test class are retrying (which is true so far), then when you add a new test, it should be harder to make the mistake because you'd have to import @Test for it.

---

core/src/test/java/google/registry/testing/AppEngineExtension.java, line 388 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

File queueFile = createFile(new File(tmpDir, "queue.xml").toPath()).toFile();

Why not just do File queueFile = new File(tmpDir, "queue.xml")?

new File() doesn't actually create the file on disk, it just creates the Java object pointing to that path. Files.createFile() is what actually creates that file.

And yes, this is not the best naming decision on Java's part. new means new File object, but create means actually create the File. Go figure.

[CydeWeys]

Reviewable status: 4 of 242 files reviewed, 2 unresolved discussions (waiting on @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/server/RegistryTestServerMain.java, line 19 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

I assume you tested this by running the server on your host?

After a little bit of refactoring this is working now.

[CydeWeys]

Should be ready for a second round of reviews now.

Reviewable status: 4 of 242 files reviewed, 2 unresolved discussions (waiting on @gbrodman, @hstonec, and @weiminyu)

[hstonec]

Reviewable status: 4 of 242 files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/testing/AppEngineExtension.java, line 388 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

new File() doesn't actually create the file on disk, it just creates the Java object pointing to that path. Files.createFile() is what actually creates that file.

And yes, this is not the best naming decision on Java's part. new means new File object, but create means actually create the File. Go figure.

Yeah, I understand that, but doesn't the asCharSink create the actual file for you?

[CydeWeys]

Reviewable status: 4 of 242 files reviewed, 2 unresolved discussions (waiting on @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/testing/AppEngineExtension.java, line 388 at r1 (raw file):

Previously, hstonec (Shicong Huang) wrote…

Yeah, I understand that, but doesn't the asCharSink create the actual file for you?

So it does. Fixed. Look like the previous person writing the code didn't know this either.

[gbrodman]

Do we have an existing bug for the SqlIntegrationMembershipTest not working? We probably should, I assume.

Reviewed 228 of 237 files at r1, 9 of 10 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/persistence/transaction/DummyJpaTransactionManagerTest.java, line 27 at r2 (raw file):

class DummyJpaTransactionManagerTest {

  @RegisterExtension final AppEngineExtension appEngine = AppEngineExtension.builder().build();

Out of curiosity why is this addition necessary?

[CydeWeys]

Yeah probably. It said TODO shicong. I'll leave that to him.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @hstonec, and @weiminyu)

[CydeWeys]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gbrodman, @hstonec, and @weiminyu)

---

core/src/test/java/google/registry/persistence/transaction/DummyJpaTransactionManagerTest.java, line 27 at r2 (raw file):

Previously, gbrodman wrote…

Out of curiosity why is this addition necessary?

It was failing in a different way without it, prior to even being able to inject the dummy transaction manager. The intent of this test is to specifically test the dummy tx manager, which is what you get when you have the AppEngineExtension but haven't specified that you want to use Cloud SQL.

[gbrodman]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hstonec and @weiminyu)

[hstonec]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @weiminyu)

[weiminyu]

Reviewed 230 of 237 files at r1, 9 of 10 files at r2, 1 of 1 files at r3, 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved