Questions about the Testcontainers usage about nomulus HOT 3 CLOSED

[copying my response here, since replying via e-mail didn't seem to do this]
Hi Sergei,

Sorry for the delayed response, thanks for getting in touch (and thanks for Testcontainers!).

Responding inline:

Sergei Egorov wrote:

Hi!

Disclaimer: I am one of the Testcontainers' maintainers

I was looking at your usage and noticed a couple of things, and thought about posting some findings:

1. DockerWebDriverRule

I see that you have the following:
https://github.com/google/nomulus/blob/3cc8d83396bc74076dc7d21d9f740b37e94471c6/core/src/test/java/google/registry/webdriver/DockerWebDriverRule.java

The question is: have you considered using the Testcontainers' Selenium module? It looks very
similar to what you have, so it is less code to maintain :)

@hstonec or @gbrodman might be able to answer this.

2. localhost usage

I noticed what you use static localhost as host:

nomulus/core/src/test/java/google/registry/webdriver/DockerWebDriverRule.java

Line 55 in 3cc8d83

"http://localhost:%d/wd/hub",

But I wanted to warn you that there are cases where it is not localhost (e.g. docker-machine, or remote Docker, or running a build inside a container). Fo
r that, Testcontainers provides Container#getContainerIpAddress(), so that the linked line can be changed to:

      url =
          new URL(
              String.format(
                  "http://%s:%d/wd/hub",
                  container.getContainerIpAddress(),
                  container.getMappedPort(CHROME_DRIVER_SERVICE_PORT)));

We're familiar with getContainerIpAddress() and are using it in other places,
this must have been an oversight. We don't currently kick off containers
remotely, but it's conceivable that someone might so we should fix this.

3. new lines

I just noticed https://github.com/google/nomulus/blob/365c5da9426f9bb7ea1a735fb33cb038cfa23603/db/src/test/java/google/registry/sql/flyway/SchemaTest.java#L53\
. Thanks a lot for reporting, we will take a look at it ASAP! Perhaps you have a simple reproducer to start with, btw?

I'm guessing that we probably don't? @hstonec would know.

4. Shutdown hook

Just FYI, there is no need [to register a shutdown hook](https://github.com/google/nomulus/blob/68887d427f8802c30f80efb13101393cb4fff14b/core/src/test/java/go\
ogle/registry/model/transaction/JpaTransactionManagerRule.java#L121) because we already have it. Even more - we have a special sidecar container that monitors t
he connection to the tests process and terminates the container if the tests are killed (e.g. with kill -9), because, when killed, JVM won't trigger the shutd
own hooks.

Good to know! We'll remove it.

---

I hope it will be helpful for your usage! :)

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#401

from nomulus.

I think we've covered everything in this issue so I'm closing this issue. Thanks for the report, @bsideup !

from nomulus.

Hi Sergei,

regarding the spurious newline, I added some comments today to the issue I opened.

Our specific problem has gone away, and I suspect it is 'fixed' by changes in transitive dependencies that testcontainer uses (I can still reproduce the problem at an older commit with locked dependencies). However I'm not sure whether this applies to small output sizes only or arbitrary-sized output.

Regards.

from nomulus.