feature-multinode-tests #70

davemoore- · 2021-02-01T22:25:58Z

Implemented a multi-node production-mode cluster for integration tests. Replaced Ant with testcontainers and docker-compose.

…s. Replaced Ant with testcontainers and docker-compose.

davemoore- · 2021-02-01T22:40:59Z

I still need to validate if this will work as-is on GitHub Actions, the implementation of which is on another PR. I'll start to merge these PRs to version branch 1.6.2.

austince

Looks really really great! Just some notes + ideas. We should also probably update the CONTRIBUTING.md once we merge this.

austince · 2021-02-01T22:37:55Z

src/test/resources/docker-compose.yml

+version: '2.2'
+services:
+  es01:
+    image: "docker.elastic.co/elasticsearch/elasticsearch:${ELASTICSEARCH_VERSION}"


Any reason not to use the latest version, which I think is around 3.7?

No reason other than it was the version used in Elastic's documented example. I can change it to the latest and commit if the tests pass afterward.

austince · 2021-02-01T22:39:23Z

src/test/java/io/zentity/resolution/AbstractITCase.java

+        Path path = Paths.get(System.getenv("BUILD_DIRECTORY"), "test-classes", "docker-compose.yml");
+        cluster = new DockerComposeContainer(new File(path.toString()))
+                .withEnv("BUILD_DIRECTORY", System.getenv("BUILD_DIRECTORY"))
+                .withEnv("ELASTICSEARCH_VERSION", System.getenv("ELASTICSEARCH_VERSION"))
+                .withEnv("ZENTITY_VERSION", System.getenv("ZENTITY_VERSION"))
+                .withExposedService(SERVICE_NAME, SERVICE_PORT,
+                        Wait.forHttp("/_cat/health")
+                                .forStatusCode(200)
+                                .withReadTimeout(Duration.ofSeconds(60)));
+        cluster.start();


This is an awesome implementation and a really nice way to expose the service. We may want to expose a debugger as well? I've found that incredibly helpful when wanting to step through the plugin code during tests.

I implemented it by setting a DEBUGGER_PORT mapping and adding -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=0.0.0.0:5050 to the ES_JAVA_OPTS.

Can you explain that debugging process more? (e.g. after committing your example, what's the expected way to make use of it). It looks easy to add, I just haven't used debuggers in Java, and it'd be good for me to have a cursory understanding of it when supporting it.

Yeah, sure thing! I've used the IntelliJ Remove JVM Debug run configuration when a debugger is exposed, which allows you to connect via the normal "debug" button and stop on breakpoints, etc.

I've found with tests, adding some arbitrary sleep before the tests start when the debugger is enabled ensures that you can connect in time.

Sounds good, I trust your judgement so I'll implement it now and explore it later.

When debugging, do you need the docker-compose file to have every node in the cluster instrumented with the new ES_JAVA_OPTS and expose its debugger port, or does the change only apply to the one node with its Elasticsearch port exposed?

I think what I'll do is turn this into an issue, instead of rushing it in via this PR as we get the patch out for #56. I'll want to make sure I do it right. I see more details around debugging in your fork, e.g. in AbstractITCase, that I'll need to understand. Or if you want to submit a PR for it, that works too!

austince · 2021-02-01T22:42:45Z

src/test/java/io/zentity/resolution/AbstractITCase.java

+    public final static int SERVICE_PORT = 9400;
+
+    // Create a singleton docker-compose cluster for all test classes.
+    private static final DockerComposeContainer cluster;


Instead of booting up this cluster in the static block, what do you think about using either a @ClassRule or in @Before / @After hooks? This will spin up multiple clusters during tests but provides much more isolation between suites.

That was my first approach before I settled on testcontainer's singleton example. I had some challenges with the other approach. I'm also trying to be cognizant of the 2,000 monthly minutes for GitHub Actions (here); having one cluster will keep the tests expeditious. I made a similar change in PR #67 to help save on minutes, too.

Ah ok, that makes sense. I was confused about the actions minutes as well, but I think the 2,000 mins/ month limit is only for private repositories.

GitHub Actions usage is free for both public repositories and self-hosted runners. For private repositories, each GitHub account receives a certain amount of free minutes and storage, depending on the product used with the account.

A similar note is on the overview page too in the "pricing" section. So I think we're good to use as many as we need!

Ah I think you're right. My usage report for GitHub Actions shows 0 minutes. It's only counting the private minutes. Generous!

I'll make another attempt at the @ClassRule / @Before / @After approach tonight.

austince · 2021-02-01T22:44:24Z

src/test/java/io/zentity/resolution/AbstractITCase.java

-            client = null;
+    // Client that communicates with the docker-compose cluster.
+    private RestClient client;
+    public RestClient client() throws IOException {


This might also be nice to set up in a @Before / @After block instead of lazily. It would allow the test runner to fail during setup and report it there if something goes wrong instead of during a test case.

austince · 2021-02-01T22:45:00Z

pom.xml

@@ -16,7 +16,7 @@
        <zentity.website>https://zentity.io</zentity.website>
        <zentity.version>1.6.1</zentity.version>
        <!-- dependency versions -->
-        <elasticsearch.version>7.10.2</elasticsearch.version>


Is this downgrade a typo?

No, I had to downgrade to <7.9.0 because the tests were failing with the multi-node cluster (as expected currently). Which also means the checks for this PR probably won't pass. I'll need to merge this with the PR on async actions before we see it fully pass.

Got it, well that's great that the cluster config is catching the bug then! Woo!

…on test cluster for all test classes.

davemoore- · 2021-02-01T23:50:36Z

Pushed a commit:

Upgraded the docker-compose file format to 3.7
Using @BeforeClass and @AfterClass to start and stop the test cluster for each test class, instead of using a singleton test cluster for all test classes.

All tests are passing on my machine.

…nstead of lazily, for each test class.

davemoore- · 2021-02-02T01:10:49Z

Pushed another commit:

The REST client is now created and destroyed on @BeforeClass and @AfterClass just like the cluster.

davemoore- · 2021-02-02T01:28:21Z

@austince I've implemented your recommendations (except for the debugger which I want to give attention to after the patch release). If it looks good to you, I'll merge this and PR #69 to a new 1.6.2 version branch. (Note: The test suite after those merges won't pass until PR #67 is merged, too.)

Edit: After reading your discussion on branching strategies, I'll follow what you proposed to see how that goes. I'll merge these PRs to master and then create the release branch from it.

austince · 2021-02-02T22:36:44Z

Nice, all looks great!

Implemented a multi-node production-mode cluster for integration test…

d3f7a2e

…s. Replaced Ant with testcontainers and docker-compose.

This was referenced Feb 1, 2021

Use a multi-node cluster in production mode for integration tests #68

Closed

Release Plan: 1.6.2 #65

Closed

austince reviewed Feb 1, 2021

View reviewed changes

davemoore- added 2 commits February 1, 2021 18:34

Upgrade to latest version of docker-compose file format

a1d0b8e

Create a test cluster for each test class, instead of using a singlet…

9853fac

…on test cluster for all test classes.

davemoore- added the release:1.6.2 label Feb 2, 2021

Create and destroy the REST client on @BeforeClass and @afterclass, i…

bd21f3d

…nstead of lazily, for each test class.

davemoore- merged commit 6e735d5 into master Feb 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature-multinode-tests #70

feature-multinode-tests #70

davemoore- commented Feb 1, 2021

davemoore- commented Feb 1, 2021

austince left a comment

austince Feb 1, 2021

davemoore- Feb 1, 2021

austince Feb 1, 2021

davemoore- Feb 1, 2021

austince Feb 1, 2021

davemoore- Feb 2, 2021

davemoore- Feb 2, 2021

austince Feb 1, 2021

davemoore- Feb 1, 2021 •

edited

Loading

austince Feb 1, 2021 •

edited

Loading

davemoore- Feb 1, 2021

davemoore- Feb 1, 2021

austince Feb 1, 2021

austince Feb 1, 2021

davemoore- Feb 1, 2021

austince Feb 1, 2021

davemoore- commented Feb 1, 2021

davemoore- commented Feb 2, 2021

davemoore- commented Feb 2, 2021 •

edited

Loading

austince commented Feb 2, 2021

feature-multinode-tests #70

feature-multinode-tests #70

Conversation

davemoore- commented Feb 1, 2021

davemoore- commented Feb 1, 2021

austince left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemoore- Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

austince Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemoore- commented Feb 1, 2021

davemoore- commented Feb 2, 2021

davemoore- commented Feb 2, 2021 • edited Loading

austince commented Feb 2, 2021

davemoore- Feb 1, 2021 •

edited

Loading

austince Feb 1, 2021 •

edited

Loading

davemoore- commented Feb 2, 2021 •

edited

Loading