jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Jclouds 46 network and image tests (#401)
Date Mon, 17 Jul 2017 06:29:40 GMT
nacx requested changes on this pull request.

Thanks, @btrishkin and apologies for the delay in the review! We've been quite busy with the
2.0.2 release.
Just some minor comments to address.

>              DEFAULT_PRIVATE_IPV4_PREFIX_SIZE);
       assertNotNull(vlanId);
       waitForVlanStatus(api(), vlanId, State.NORMAL, 30 * 60 * 1000, "Error - unable to deploy
vlan");
    }
 
    @Test
+   public void testAddPublicIPv4Block() {
+      publicIpv4BlockId = api().addPublicIpBlock(PREPARED_NETWORK_DOMAIN_ID);
+      assertNotNull(publicIpv4BlockId);
+   }
+
+   @Test(dependsOnMethods = "testAddPublicIPv4Block")
+   public void testListPublicIPv4AddressBlocks() {
+      PagedIterable<PublicIpBlock> ipBlockList = api().listPublicIPv4AddressBlocks(PREPARED_NETWORK_DOMAIN_ID);
+      assertTrue(!ipBlockList.isEmpty());
+      assertEquals(ipBlockList.last().get().first().get().size(), 2);
+      assertEquals(ipBlockList.last().get().first().get().networkDomainId(), PREPARED_NETWORK_DOMAIN_ID);
+   }
+
+   @Test(dependsOnMethods = "testListPublicIPv4AddressBlocks")

This could depend on the "add" test to maximize the chances of this test being executed.

> +
+   @Test(dependsOnMethods = "testGetPublicIPv4AddressBlocks")
+   public void testCreateNatRule() {
+      natRuleId = api()
+            .createNatRule(PREPARED_NETWORK_DOMAIN_ID, PREPARED_PRIVATE_IPV4_ADDRESS, publicIpBlock.baseIp());
+      assertNotNull(natRuleId);
+   }
+
+   @Test(dependsOnMethods = "testCreateNatRule")
+   public void testListNatRules() {
+      PagedIterable<NatRule> natRulesList = api().listNatRules(PREPARED_NETWORK_DOMAIN_ID);
+      assertTrue(!natRulesList.isEmpty());
+      assertEquals(natRulesList.last().get().first().get().networkDomainId(), PREPARED_NETWORK_DOMAIN_ID);
+   }
+
+   @Test(dependsOnMethods = "testListNatRules")

Also depend on the "add" test.

> +
+   @Test(dependsOnMethods = "testCreateNatRule")
+   public void testListNatRules() {
+      PagedIterable<NatRule> natRulesList = api().listNatRules(PREPARED_NETWORK_DOMAIN_ID);
+      assertTrue(!natRulesList.isEmpty());
+      assertEquals(natRulesList.last().get().first().get().networkDomainId(), PREPARED_NETWORK_DOMAIN_ID);
+   }
+
+   @Test(dependsOnMethods = "testListNatRules")
+   public void testGetNatRule() {
+      NatRule natRule = api().getNatRule(natRuleId);
+      assertNotNull(natRule);
+      assertEquals(natRule.networkDomainId(), PREPARED_NETWORK_DOMAIN_ID);
+   }
+
+   @Test(dependsOnMethods = "testGetNatRule")

Annotate cleanup methods with `alwaysRun = true` to make sure they're executed even the tests
they depend on have failed. This way we avoid leaking resources used in tests.

> @@ -123,4 +213,13 @@ private NetworkApi api() {
       return api.getNetworkApi();
    }
 
+   private FirewallRule findById(List<FirewallRule> collection, String id) {
+      FirewallRule result = null;
+      for (FirewallRule rule : collection) {
+         if (rule.id().equals(id)) {
+            result = rule;

Just `return rule`.

> @@ -217,4 +225,15 @@ protected void assertBodyContains(RecordedRequest recordedRequest,
String expect
          throw Throwables.propagate(e);
       }
    }
+
+   private List<NameValuePair> parsePath(String fullPath) {
+      return URLEncodedUtils.parse(fullPath, Charset.defaultCharset(), '?', '&');
+   }
+
+   private void assertParameters(List<NameValuePair> parsedPath, List<NameValuePair>
parsedRequest) {
+      assertEquals(parsedPath.size(), parsedRequest.size());
+      for (NameValuePair parameter : parsedPath) {
+         assertTrue(parsedRequest.contains(parameter));
+      }
+   }

I don't really get the value in comparing the requests like this. Can't just we compare the
strings and avoid the need of adding an additional dependency just for this?
Also, this check does not guarantee paths are the same; it does not check the order of the
value pairs. Consider just comparing the requests path strings, as we do in all other providers.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/401#pullrequestreview-50242150
Mime
View raw message