[PAN-2577] Integration Integration test(s) on p2p of 'net_services' #1402

smatthewenglish · 2019-05-06T05:49:07Z

PR description

An acceptance tests for PAN‌-2577.

Fixed Issue(s)

Ensures the proper performance of the net_services endpoint.

shemnon · 2019-05-06T14:04:20Z

To answer the question in the PR description, that is exactly what we are trying to fix. There was a unit tests that mocked things up differently that passed at a unit level. What we need is a non-mocked test where pantheon is stood up and we see all the ports we opened in net_services.

We need a test that fails against revision d5dd1d4e but passes after revision 105dc5b5

mbaxter · 2019-05-13T16:18:00Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+    assertThat(expectation.get("jsonrpc").get("port")).isEqualTo(result.get("jsonrpc").get("port"));
+    assertThat(expectation.get("ws").get("host")).isEqualTo(result.get("ws").get("host"));
+    assertThat(expectation.get("ws").get("port")).isEqualTo(result.get("ws").get("port"));
+    assertThat(expectation.get("p2p").get("host")).isEqualTo(result.get("p2p").get("host"));


You're not checking the p2p port value here

mbaxter · 2019-05-13T16:19:02Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+    expectation.put("jsonrpc", constituentMap);
+    expectation.put("ws", constituentMap);
+    expectation.put("p2p", constituentMap);
+    assertThat(expectation.get("jsonrpc").get("host")).isEqualTo(result.get("jsonrpc").get("host"));


The format of assertThat should be: assertThat(actual).isEqualTo(expected) - these checks should be reformatted.

mbaxter · 2019-05-13T16:19:16Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+  }
+
+  @Test
+  public void adminAddPeerForcesConnection() {


This method should be renamed

mbaxter · 2019-05-13T16:21:56Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+        new HashMap() {
+          {
+            put("host", "127.0.0.1");
+            put("port", "0");


Expecting a port of "0" for everything is a little confusing. Can you explicitly configure the json-rpc and ws ports to distinct values?

mbaxter · 2019-05-13T16:22:37Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+    noDiscoveryCluster.start(nodeA, nodeB);
+  }
+
+  @Test


Can you add a test case where all net services (json-rpc, ws, and p2p) are disabled?

mbaxter · 2019-05-14T22:07:21Z

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/net/NetServicesTransaction.java

+    }
+    return (netServicesResponse != null ? netServicesResponse.getResult() : null) != null
+        ? netServicesResponse.getResult()
+        : null;


Looks like you've got an extra ternary operator here 😄

...pantheon/tests/acceptance/dsl/condition/net/ExpectNetServicesReturnsAllServicesAsActive.java

mbaxter · 2019-05-15T13:56:08Z

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/JsonRequestFactories.java

@@ -68,6 +71,10 @@ public AdminJsonRpcRequestFactory admin() {
    return admin;
  }

+  public NetServicesJsonRpcRequestFactory netServices() {


What do you think about renaming NetServicesJsonRpcRequestFactory to something like CustomNetJsonRpcRequestFactory. Then if we end up expanding the "net_" api in the future, we can add more methods to this factory. Then this method would become customNet().

mbaxter · 2019-05-15T13:59:25Z

...tech/pegasys/pantheon/tests/acceptance/dsl/transaction/NetServicesJsonRpcRequestFactory.java

+  public Request<?, NetServicesResponse> netServices() {
+
+    return new Request<>(
+        "net_services", Collections.EMPTY_LIST, web3jService, NetServicesResponse.class);


Suggested change

"net_services", Collections.EMPTY_LIST, web3jService, NetServicesResponse.class);

"net_services", Collections.emptyList(), web3jService, NetServicesResponse.class);

If you use emptyList() here, you can remove the annotation @SuppressWarnings({"unchecked", "rawtypes"})

mbaxter · 2019-05-15T14:01:44Z

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/net/NetServicesTransaction.java

+      Request<?, NetServicesJsonRpcRequestFactory.NetServicesResponse> request =
+          netServicesJsonRpcRequestFactory.netServices();
+      netServicesResponse = request.send();
+    } catch (final Exception ignored) {


You shouldn't ignore an exception here, you should throw.

mbaxter · 2019-05-15T14:03:32Z

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/net/NetServicesTransaction.java

+      netServicesResponse = request.send();
+    } catch (final Exception ignored) {
+    }
+    return netServicesResponse != null ? netServicesResponse.getResult() : null;


You shouldn't need to check for null. Just move the return statement into the try block and throw the exception instead of ignoring it and you can return getResult() directly.

mbaxter · 2019-05-15T14:07:07Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+    final ClusterConfiguration clusterConfiguration =
+        new ClusterConfigurationBuilder().setAwaitPeerDiscovery(false).build();
+    noDiscoveryCluster = new Cluster(clusterConfiguration, net);
+    nodeA = pantheon.createArchiveNodeWithDiscoveryDisabledAndAdmin("nodeA");


Can you add a utility for creating a node with all net services turned on?

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

mbaxter · 2019-05-15T14:18:48Z

.../src/test/java/tech/pegasys/pantheon/tests/acceptance/jsonrpc/NetServicesAcceptanceTest.java

+    nodeB = pantheon.createArchiveNodeNetServicesDisabled("nodeB");
+    noDiscoveryCluster.start(nodeA, nodeB);
+
+    nodeA.verify(net.netServicesNotActive());


You could create a generic method that can be configured with expected services: net.netServices(List<String> expectedServices)

mbaxter · 2019-05-16T22:51:54Z

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/net/NetServicesTransaction.java

+      CustomNetJsonRpcRequestFactory.NetServicesResponse netServicesResponse = request.send();
+      netServicesActive = netServicesResponse.getResult();
+    } catch (final Exception e) {
+      LOG.error("Error parsing response to 'net_services' json-rpc request.", e);


This should throw, not log an error

Suggested change

LOG.error("Error parsing response to 'net_services' json-rpc request.", e);

throw new RuntimeException(e);

mbaxter · 2019-05-16T22:52:31Z

.../java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/net/NetServicesTransaction.java

+    } catch (final Exception e) {
+      LOG.error("Error parsing response to 'net_services' json-rpc request.", e);
+    }
+    return netServicesActive;


If the catch rethrows, you don't need this extra return statement

mbaxter · 2019-05-16T22:56:29Z

...pantheon/tests/acceptance/dsl/condition/net/ExpectNetServicesReturnsAllServicesAsActive.java

+    assertThat(NetworkUtility.isValidPort(p2pPort) || jsonRpcPort == 0).isTrue();
+  }
+
+  public static class ExpectNetServicesReturnsNoServicesAsActiveX implements Condition {


Suggested change

public static class ExpectNetServicesReturnsNoServicesAsActiveX implements Condition {

public static class ExpectNetServicesReturnsOnlyJsonRpcActive implements Condition {

mbaxter · 2019-05-16T22:57:12Z

...pantheon/tests/acceptance/dsl/condition/net/ExpectNetServicesReturnsAllServicesAsActive.java

+    assertThat(NetworkUtility.isValidPort(p2pPort) || jsonRpcPort == 0).isTrue();
+  }
+
+  public static class ExpectNetServicesReturnsNoServicesAsActiveX implements Condition {


Can you make this an independent class in a separate file rather than an internal class?

mbaxter · 2019-05-16T22:58:41Z

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/JsonRequestFactories.java

@@ -68,6 +71,10 @@ public AdminJsonRpcRequestFactory admin() {
    return admin;
  }

+  public CustomNetJsonRpcRequestFactory netServices() {


Can you make the naming for CustomNetJsonRpcRequestFactory-related variables and methods consistent? So this would be: customNet()

This method name still needs to be updated

mbaxter · 2019-05-16T23:00:18Z

...a/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/CustomNetJsonRpcRequestFactory.java

+    this.web3jService = web3jService;
+  }
+
+  public Request<?, NetServicesResponse> customNet() {


Suggested change

public Request<?, NetServicesResponse> customNet() {

public Request<?, NetServicesResponse> netServices() {

mbaxter · 2019-05-16T23:00:47Z

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/JsonRequestFactories.java

@@ -34,13 +35,15 @@ public JsonRequestFactories(
      final PermissioningJsonRpcRequestFactory perm,
      final AdminJsonRpcRequestFactory admin,
      final EeaJsonRpcRequestFactory eea,
+      final CustomNetJsonRpcRequestFactory netServices,


Suggested change

final CustomNetJsonRpcRequestFactory netServices,

final CustomNetJsonRpcRequestFactory customNet,

mbaxter · 2019-05-16T23:01:00Z

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/JsonRequestFactories.java

@@ -25,6 +25,7 @@
  private final PermissioningJsonRpcRequestFactory perm;
  private final AdminJsonRpcRequestFactory admin;
  private final EeaJsonRpcRequestFactory eea;
+  private final CustomNetJsonRpcRequestFactory netServices;


Suggested change

private final CustomNetJsonRpcRequestFactory netServices;

private final CustomNetJsonRpcRequestFactory customNet;

mbaxter

You missed one method name update, but otherwise looks good! Please update before merging.

mbaxter · 2019-05-17T14:39:35Z

...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/transaction/JsonRequestFactories.java

@@ -68,6 +71,10 @@ public AdminJsonRpcRequestFactory admin() {
    return admin;
  }

+  public CustomNetJsonRpcRequestFactory netServices() {


This method name still needs to be updated

smatthewenglish added the work in progress Work on this pull request is ongoing label May 6, 2019

smatthewenglish force-pushed the net_services branch 3 times, most recently from 9f7e06d to f8e6392 Compare May 6, 2019 10:17

smatthewenglish force-pushed the net_services branch 8 times, most recently from e41902e to 64efbdc Compare May 13, 2019 11:15

smatthewenglish removed the work in progress Work on this pull request is ongoing label May 13, 2019

mbaxter reviewed May 13, 2019

View reviewed changes

smatthewenglish force-pushed the net_services branch 4 times, most recently from 72e6872 to d3372f0 Compare May 14, 2019 16:46

smatthewenglish requested a review from mbaxter May 14, 2019 17:28

smatthewenglish force-pushed the net_services branch from 0727e0b to fed6f6e Compare May 14, 2019 21:09

mbaxter reviewed May 14, 2019

View reviewed changes

smatthewenglish requested a review from mbaxter May 14, 2019 22:19

mbaxter reviewed May 15, 2019

View reviewed changes

smatthewenglish force-pushed the net_services branch 3 times, most recently from 2aa359d to e71f7ab Compare May 16, 2019 22:46

mbaxter reviewed May 16, 2019

View reviewed changes

smatthewenglish requested a review from mbaxter May 16, 2019 23:26

mbaxter approved these changes May 17, 2019

View reviewed changes

smatthewenglish added 27 commits May 17, 2019 12:35

~

c874431

x

22092fe

Z

2ece452

InetAddresses

046555f

~

7749958

update

e7aad8b

~

c886926

comment

bdf34a9

d

1339489

update

6d70289

ud

c3857d7

xd 1

370ca35

shouldNotDisplayDisabledServices

8c9c34b

~

9098430

netServicesOnlyJsonRpcEnabled

674eeb0

~

04682b7

x

aa0b78a

setPushEnabled

1e1bdcc

PIE-1606

f51b1c2

x

00456aa

x

6aecd20

c

2327a71

bvb

308f9df

customNet

8369f3b

netServices

22cb9f2

customNet

9d203b9

999

bfd2078

smatthewenglish force-pushed the net_services branch from 6de788a to bfd2078 Compare May 17, 2019 16:35

Merge branch 'master' into net_services

048343f

smatthewenglish merged commit 8b26d3a into PegaSysEng:master May 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAN-2577] Integration Integration test(s) on p2p of 'net_services' #1402

[PAN-2577] Integration Integration test(s) on p2p of 'net_services' #1402

smatthewenglish commented May 6, 2019 •

edited

Loading

shemnon commented May 6, 2019

mbaxter May 13, 2019

mbaxter May 13, 2019

mbaxter May 13, 2019

mbaxter May 13, 2019

mbaxter May 13, 2019

mbaxter May 14, 2019

mbaxter May 15, 2019

mbaxter May 15, 2019

mbaxter May 15, 2019

mbaxter May 15, 2019

mbaxter May 15, 2019

mbaxter May 15, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter May 17, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

mbaxter left a comment

mbaxter May 17, 2019

	"net_services", Collections.EMPTY_LIST, web3jService, NetServicesResponse.class);
	"net_services", Collections.emptyList(), web3jService, NetServicesResponse.class);

	LOG.error("Error parsing response to 'net_services' json-rpc request.", e);
	throw new RuntimeException(e);

	public static class ExpectNetServicesReturnsNoServicesAsActiveX implements Condition {
	public static class ExpectNetServicesReturnsOnlyJsonRpcActive implements Condition {

	public Request<?, NetServicesResponse> customNet() {
	public Request<?, NetServicesResponse> netServices() {

	final CustomNetJsonRpcRequestFactory netServices,
	final CustomNetJsonRpcRequestFactory customNet,

	private final CustomNetJsonRpcRequestFactory netServices;
	private final CustomNetJsonRpcRequestFactory customNet;

[PAN-2577] Integration Integration test(s) on p2p of 'net_services' #1402

[PAN-2577] Integration Integration test(s) on p2p of 'net_services' #1402

Conversation

smatthewenglish commented May 6, 2019 • edited Loading

PR description

Fixed Issue(s)

shemnon commented May 6, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smatthewenglish commented May 6, 2019 •

edited

Loading