-
Notifications
You must be signed in to change notification settings - Fork 321
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
Add testcontainers based integration test #57
Conversation
Hey @bsideup, I have two things left I'm not quite sure how to solve, see the two The biggest problem is the lifecycle of the Also, I didn't quite get debugging to work. |
Hi @felixbarny, Let me take a look at TODOs and the PR in general 👍 |
.withDockerfileFromBuilder(builder -> builder | ||
.from("tomcat:9") | ||
.run("rm -rf /usr/local/tomcat/webapps/*") | ||
.copy("ROOT.war", "/usr/local/tomcat/webapps/ROOT.war") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to mount it as a file instead of baking into your image. Otherwise you will create a new image every test run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check .withFileSystemBind()
.expose(8080, 8000) | ||
.entryPoint("catalina.sh", "jpda", "run") | ||
) | ||
// TODO chicken egg problem here: tests require the war to be present, which is built via mvn package, but mvn package executes the tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we were doing the same thing in ZeroTurnaround, we ended up with Groovy-based setup to avoid building test apps at all.
i.e. https://github.com/bsideup/javaagent-boilerplate/tree/master/src/test + https://gist.github.com/bsideup/70091d2f762b8bc7d9255260c0b5ed15 to test Servet-based app.
You can also adjust Maven's lifecycle to execute war packaging before the test.
Also, AFAIK maven-failsafe-plugin
will run after WAR packaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a groovy based setup sounds good. But I also want to be able to test applications deployed in a traditional way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, then maven-failsafe-plugin
is your friend. The only thing is that you loose IDE friendliness of your tests (you can't run them from your IDE)
Ofc there is another option to use Gradle, and use "Delegate build/test actions to Gradle" in your IDE. This way you can have your test app as a dependency and Gradle will automatically build it for you (if not built) before running the test, and you will be able to run the test from IDE as well, but I understand that changing the build system is not what people usually want to do :D Just FYI-ing you about the possibility and advantages
public static void beforeClass() { | ||
mockServerContainer.getClient().when(request("/v1/transactions")).respond(HttpResponse.response().withStatusCode(200)); | ||
mockServerContainer.getClient().when(request("/v1/errors")).respond(HttpResponse.response().withStatusCode(200)); | ||
c.followOutput(new Slf4jLogConsumer(logger)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also use fluent .withLogConsumer()
|
||
private static final Logger logger = LoggerFactory.getLogger(ServletIntegrationTest.class); | ||
|
||
@ClassRule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit's @ClassRule
doesn't work with abstract classes, I would suggest removing the rules and adding:
static {
Stream.of(c, mockServerContainer).parallel().forEach(GenericContainer::start);
}
This "pattern" is also called "Singleton container" :D
</dependency> | ||
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>testcontainers</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there is also a BOM: org.testcontainers:testcontainers-bom:1.7.0
in case you plan using other containers like PostgreSQL, Selenium, etc
.from("tomcat:9") | ||
.run("rm -rf /usr/local/tomcat/webapps/*") | ||
.copy("ROOT.war", "/usr/local/tomcat/webapps/ROOT.war") | ||
// TODO debugging does not work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember fighting with JVM debugging inside Docker (not related to Testcontainers), but I don't remember the config anymore :D I just remember that JVM's debugging protocol is not NAT friendly
What I could recommend is to do docker run -p 18000:8000 -e JPDA_ADDRESS=... -e JPDA_TRANSPORT=... tomcat:9 catalina.sh jpda run
and play with parameters until you can debug it on port 18000 (while it is 8000 inside the container)
You can also manually add a fixed port mapping when you debug (but I would suggest to protect it with env variable, system property or something else to avoid port conflicts on CI environments)
Third option would be to attempt to start a SocatContainer
and gracefully handle the failure because of the port conflict, so that it will not fail on CI envs, I can provide you an example if you want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of the Socat container would be great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will provide one later today :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it is a problem with the tomcat:9
container. It works with tomcat:8
¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoa :D
@felixbarny and don't forget to turn on Docker on Travis ;) sudo: true
services:
- docker |
Codecov Report
@@ Coverage Diff @@
## master #57 +/- ##
============================================
+ Coverage 75.77% 76.01% +0.23%
- Complexity 723 724 +1
============================================
Files 78 78
Lines 2621 2643 +22
Branches 233 240 +7
============================================
+ Hits 1986 2009 +23
- Misses 497 501 +4
+ Partials 138 133 -5
Continue to review full report at Codecov.
|
I really don't get why tests are failing on Jenkins. Instead of returning the Hello World jsp, Tomcat responds with a 404, even though the war seems to be deployed successfully.
The relative referencing of The connection to the tomcat container doesn't seem to be the problem, otherwise tomcat wouldn't be able to respond with a 404. The on |
@felixbarny or, actually, I see |
I suspect something goes wrong when mounting the file. I even downloaded the war from the workspace and executed locally with success. And assert on test startup that the war file exists and that it has content: https://github.com/elastic/apm-agent-java/pull/57/files#diff-86c386663b0d434b566084161d7f2a22R94.
I'm actually not sure if the jenkins slaves are bare metal, VMs or dockerized. I'll ask. |
@felixbarny try adding a |
Makes sense! Tomcat listening on a socket does not mean that the application has already been deployed! Will have to wait until there are free build slaves.. |
Nope
The deployment of the war is also suspiciously fast:
Locally, it takes around 1.5 seconds. |
Note that is also says
When executing locally, it says |
Also The Jenkins slaves are either EC2 instances or bare metal hosts and are not dockerized. |
@felixbarny out of curiosity - could you please try to deploy your app under a context (eg "app.war")? |
Still no luck. The
|
@felixbarny there is a..... hm... questionable behaviour of Docker, when you mount non-existing file it mounts it as an empty folder. One clue - does your CI environment runs as Docker Swarm? |
Do you mean if the workers have docker swarm installed or are managed by docker swarm? As they are not docker containers I don't think they are managed by docker swarm. |
@felixbarny more like if Swarm is initialized on the machines with agents...
This line (and |
Good point. But then it's strange that @elasticdog could you comment on wether docker images executed in the CI environment may be scheduled to other machines via docker swarm? Is there anything special in our deployment which could cause mounting a file to fail? |
@felixbarny @bsideup the I'm not personally a Docker expert, but I don't know of anything off-hand that would be unique to our infrastructure that would cause this to fail. Can you point us to the docker command that you're running to mount the file so other members on the infra team can chime in? |
Seems like testcontainers does not create a Dockerfile but uses the docker socket api. This is what is sent to the
|
@elasticdog @felixbarny one can easily verify the environment in isolation by running something like: docker run \
-p 8080:8080 \
-it --rm \
-v /var/lib/jenkins/workspace/elastic+apm-agent-java+pull-request+multijob-test/src/github.com/elastic/apm-agent-java/apm-agent-plugins/apm-servlet-plugin-it/simple-webapp-integration-test/../simple-webapp/target/ROOT.war:/usr/local/tomcat/webapps/ROOT.war \
tomcat:8 |
40888bb
to
875810c
Compare
@elasticdog can you or someone from the infra team help to investigate on this? Steps:
Problem: |
|
||
private static final Logger logger = LoggerFactory.getLogger(ServletIntegrationTest.class); | ||
|
||
private static final String pathToWar = "../simple-webapp/target/ROOT.war"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to give the full path to docker for mounting a directory.
Example:
❯ docker run --rm -v ../shared:/app -w /app -ti alpine
docker: Error response from daemon: create ../shared: "../shared" includes invalid characters for a local volume name, only "[a-zA-Z0-9][a-zA-Z0-9_.-]" are allowed. If you intended to pass a host directory, use absolute path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming withFileSystemBind
doesn't automatically expand the path for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But let me check if we normalize it or not (we should)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realised this works on travis ci, so obviously it must be somehow :)
Not sure if my comment about the full path or not is related. However the path in the comment above doesn't seem to exist on the worker node.
The closest thing I can find is an empty directory at |
Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
to see if the application has started properly Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
One of those problems which resolve themselves over time... But that's actually very good news, even if I have to wait for a new docker-java release. Funny thing is that the new docker-java version does not log the |
7ebb8a0
to
f2f80c2
Compare
Even with the fact that the problem is in docker-java and not in Testcontainers I still feel sorry about having you to debug the problem and me almost unable to help 😅 Actually, in Testcontainers 1.7.1 we replaced docker-java's NettyCmdFactory with our own which uses daemon threads instead (long awaited feature request from Spring guys), and removed the packages logger. While it might be helpful in some rare cases like yours, a lot of users (very unfortunately) doesn't know how to configure the logging and were complaining about HEX dumps in their tests, this is why we decided to drop the logger. |
@felixbarny FYI it seems that you can easily workaround the docker-java's bug by doing something like the following: File tempWarFile = new File(System.getProperty("user.home"), UUID.randomUUID() + ".war");
tempWarFile.deleteOnExit();
Files.copy(warFile, tempWarFile.toPath());
withFileSystemBind(tempWarFile.getAbsolutePath(), "/app.war") Assuming that user's home directory will not have |
@felixbarny another option would be to start Tomcat and do: tomcat.copyFileToContainer(MountableFile.forHostPath(warFilePath), "/usr/local/tomcat/webapps/ROOT.war") The reason why it works because it will transfer the file as InputStream to docker daemon instead of telling the daemon to use one of the files on the daemon's file system :) |
Useful for integration tests to ensure the transactions have been reported Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
This works around a bug in docker-java which encodes the plus sign to a space when binding a file to the container. Also restructures the integration tests into a top-level integration-tests module as it felt a bit awkward as a apm-agent-plugins submodule. It's not a plugin and we want to avoid packaging the tests applications like simple-webapp into the main distribution. Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
@felixbarny snap! Apparently, there is also a bug in Testcontainers in |
I'm thinking about deploying the wars via the tomcat manager API (https://tomcat.apache.org/tomcat-9.0-doc/manager-howto.html#Deploy_A_New_Application_Archive_(WAR)_Remotely). That way I can reuse the container across different tests. But that's something for the future... @bsideup what do you think about that approach? |
- Deploy war withFileSystemBind instead of copyFileToContainer as that is much faster (1s vs 8s) Signed-off-by: Felix Barnsteiner <felix.barnsteiner@elastic.co>
@felixbarny we had some major issues with such approach:
Also, Tomcat + deployment adds to the test execution time. |
Thx for the suggestions. Whenever feasible, I'll test with a simple Groovy Jetty script. But part of what I want to verify with the integration tests is that the agent works properly with a specific application server in a specific version. YEY, the tests are green :D |
TODO: war creation and debugging
closes #20