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
