test: Improve integration test framework #295

IgorPerikov · 2018-03-03T16:10:49Z

according to this issue #106
this pull request

introduce testcontainers framework usage for all suitable tests rewrite all tests, so they use their separate etcd clusters and nodes
remove old script
fixed travis build command (I hope 😄 ) so It doesn't launch old script anymore
fixed README section about launching tests

IgorPerikov · 2018-03-03T16:13:18Z

jetcd-core/src/test/java/com/coreos/jetcd/internal/impl/ClusterClientTest.java

+    thirdNode.start();
+    endpoints = TestUtil.buildClientEndpoints(firstNode, secondNode, thirdNode);
+    peerUrls = TestUtil.buildPeerEndpoints(firstNode, secondNode, thirdNode);
+    TimeUnit.SECONDS.sleep(5);


Client was saying, that cluster is unhealthy. So I gave him some time here

fanminshi · 2018-03-05T21:11:24Z

@IgorPerikov Thanks for the pr. I give this a thought. If we can wrap all the etcd containers creation logic into a new class called Cluster can probably a better abstraction over just launch each node by itself. I am suggestion the following interface:

/**
 * Cluster is an etcd cluster.
 */
public interface Cluster {

  // randClient randomly picks a etcd client within the cluster.
  Client randClient();

  // client gets the member client based on member idx within the cluster.
  Client client(int i);

  // terminate terminates the etcd cluster.
  void terminate();
}

Also a factory class to create it.

public class ClusterFactory {
  // newCluster returns a launched cluster with client connection
  // for each cluster member.
  public static Cluster newCluster(int size) {
    return null;
  }
}

Let me know what you think. also the above idea is based on the integration test framework from etcd. ref: https://github.com/coreos/etcd/blob/master/integration/cluster.go#L1000

cc/ @lburgazzoli

IgorPerikov · 2018-03-06T14:30:13Z

I'm not exactly agree with Cluster interface, but it's a good idea to build another abstraction, which will handle all this infrastructure logic. I'll develop the concept and show it to you later

IgorPerikov · 2018-03-06T19:16:10Z

@fanminshi I've pushed draft of the cluster abstraction. There might be still some small mistakes/bad namings or so, but general idea is there. More about some design decisions I've made:

Since many of the tests manipulate over endpoints themselves, I decided not to hide endpoints inside cluster abstraction, I don't think that rewriting logic of tests is good goal here, but eventually it might be a good idea to hide them too
As I can see there is no point in methods like randClient() and client(int idx), because we operate with whole cluster anyway
Simple factory class/no fascinate builder etc: since all the tests requires either single node or 3 node cluster there is no big point in implementing extremely flexible method which can create custom size cluster, looks like overengineering for me
There is some custom annotations for test frameworks which will call close() methods on marked objects, but since it's not very common libraries I decided to kept the explicit cluster.close() calls in @AfterXXX methods

fanminshi · 2018-03-07T00:08:33Z

I'm not exactly agree with Cluster interface
Those are copy/pasted from etcd's Cluster. It is okay that jetcd's abstraction is different. I just want us to start thinking about the new interface.

As I can see there is no point in methods like randClient() and client(int idx), because we operate with whole cluster anyway
Yeah, for now those are not useful. we don't have to include those.

fanminshi · 2018-03-07T00:12:55Z

jetcd-core/src/test/java/com/coreos/jetcd/internal/impl/KVTest.java

@@ -221,4 +227,9 @@ public void testNestedTxn() throws Exception {
    test.assertEquals(getResp2.getKvs().size(), 1);
    test.assertEquals(getResp2.getKvs().get(0).getValue().toStringUtf8(), oneTwoThree.toStringUtf8());
  }
+
+  @AfterTest
+  public void stop() throws IOException {


maybe use public void tearDown() instead? that is being used in everywhere else i think.

fanminshi · 2018-03-07T00:13:40Z

jetcd-core/src/test/java/com/coreos/jetcd/internal/impl/MaintenanceTest.java

@@ -117,4 +121,9 @@ public void testMoveLeader() throws ExecutionException, InterruptedException {
      client.getMaintenanceClient().moveLeader(followers.get(0)).get();
    }
  }
+
+  @AfterTest
+  public void stopContainers() throws IOException {


use public void tearDown()?

fanminshi · 2018-03-07T00:16:41Z

@IgorPerikov the new interface looks great! just some nits then lgtm. thanks for the great work!

fanminshi · 2018-03-07T00:20:54Z

Also after you have addressed my comments/suggestions, could you also rebase the commits? e.g
a commit for updating readme.md, a commit for update travis, a commit for adding EtcdCluster and its related stuff, a commit for updating tests using EtcdCluster, and etc. I hope you get the idea.

If you can follow the commit style guide line from here will be great: https://github.com/coreos/etcd/blob/master/CONTRIBUTING.md#format-of-the-commit-message

fanminshi · 2018-03-08T17:31:48Z

@IgorPerikov kindly ping?

IgorPerikov · 2018-03-08T17:55:41Z

@fanminshi Hi! I had a day off, and I'm gonna do the rest of the pr-related work tonight, stay tuned!

use testcontainers-java for cleaner test environment Fixes #106

Fixes #106

run_etcd_docker no longer needed, since testcontainers used for testing Fixes #106

Fixes #106

IgorPerikov · 2018-03-08T20:29:05Z

@fanminshi done

IgorPerikov commented Mar 3, 2018

View reviewed changes

fanminshi reviewed Mar 7, 2018

View reviewed changes

fanminshi changed the title ~~Improve integration test framework~~ test: Improve integration test framework Mar 7, 2018

IgorPerikov added 5 commits March 8, 2018 22:07

build: add testcontainers-java dependency

d2193f9

test: introduce EtcdCluster abstraction

24714d4

use testcontainers-java for cleaner test environment Fixes #106

test: rewrite tests using EtcdCluster

360a964

Fixes #106

travis_ci: remove unnecessary script from ci pipeline

44cf027

run_etcd_docker no longer needed, since testcontainers used for testing Fixes #106

etc: remove unused script and update readme

1645678

Fixes #106

fanminshi merged commit 5ceec9c into etcd-io:master Mar 8, 2018

fanminshi mentioned this pull request Apr 10, 2018

untangle test dependency in AuthClientTest. #105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Improve integration test framework #295

test: Improve integration test framework #295

IgorPerikov commented Mar 3, 2018

IgorPerikov Mar 3, 2018

fanminshi commented Mar 5, 2018 •

edited

Loading

IgorPerikov commented Mar 6, 2018

IgorPerikov commented Mar 6, 2018

fanminshi commented Mar 7, 2018 •

edited

Loading

fanminshi Mar 7, 2018

fanminshi Mar 7, 2018

fanminshi commented Mar 7, 2018

fanminshi commented Mar 7, 2018 •

edited

Loading

fanminshi commented Mar 8, 2018

IgorPerikov commented Mar 8, 2018 •

edited

Loading

IgorPerikov commented Mar 8, 2018

test: Improve integration test framework #295

test: Improve integration test framework #295

Conversation

IgorPerikov commented Mar 3, 2018

IgorPerikov Mar 3, 2018

Choose a reason for hiding this comment

fanminshi commented Mar 5, 2018 • edited Loading

IgorPerikov commented Mar 6, 2018

IgorPerikov commented Mar 6, 2018

fanminshi commented Mar 7, 2018 • edited Loading

fanminshi Mar 7, 2018

Choose a reason for hiding this comment

fanminshi Mar 7, 2018

Choose a reason for hiding this comment

fanminshi commented Mar 7, 2018

fanminshi commented Mar 7, 2018 • edited Loading

fanminshi commented Mar 8, 2018

IgorPerikov commented Mar 8, 2018 • edited Loading

IgorPerikov commented Mar 8, 2018

fanminshi commented Mar 5, 2018 •

edited

Loading

fanminshi commented Mar 7, 2018 •

edited

Loading

fanminshi commented Mar 7, 2018 •

edited

Loading

IgorPerikov commented Mar 8, 2018 •

edited

Loading