You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/06/22 10:58:36 UTC
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/742
DefaultAzureArmNetworkCreator improvements
* Don’t use RETURNS_DEEP_STUBS as causes non-deterministic test failure
on my dev machine (ClassCastException when calling `thenReturn(...)`!)
* Tidy logging
* Assert create() calls are made
* Minor renames (e.g. _PREFIX to indicate DEFAULT_RESOURCE_GROUP constant is just the naming prefix)
Note the aim of the logging improvements is to ensure there is just one log.info message per provision request to say we're messing with the user's templateOptions, and one info message if we're creating default resource group, and one info message if creating default network/subnet. If we don't touch the user's config then we just log at debug.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server pr739-minor-tidy
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/742.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #742
----
commit d198e28bef812ab542295d0dd217aa6a7344d6e3
Author: Aled Sage <al...@gmail.com>
Date: 2017-06-22T10:55:46Z
DefaultAzureArmNetworkCreator improvements
* Don’t use RETURNS_DEEP_STUBS as causes non-deterministic test failure
on my dev machine (ClassCastException when calling `thenReturn(...)`!)
* Tidy logging
* Assert create() calls are made
* Minor renames (e.g. _PREFIX to indicate DEFAULT_RESOURCE_GROUP
constant is just the naming prefix)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
LGTM, also see the comments I just added to https://github.com/apache/brooklyn-server/pull/739
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/742#discussion_r123482551
--- Diff: locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreatorTest.java ---
@@ -90,62 +97,44 @@ public void testPreExisting() {
verify(azureComputeApi).getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME);
Map<String, Object> templateOptions = configBag.get(TEMPLATE_OPTIONS);
- Map<String, Object> ipOptions = (Map<String, Object>)templateOptions.get("ipOptions");
+ Map<?, ?> ipOptions = (Map<?, ?>)templateOptions.get("ipOptions");
assertEquals(ipOptions.get("subnet"), TEST_SUBNET_ID);
assertEquals(ipOptions.get("allocateNewPublicIp"), true);
}
@Test
- public void testVanilla() {
- //Setup config bag
- ConfigBag configBag = ConfigBag.newInstance();
- configBag.put(CLOUD_REGION_ID, TEST_LOCATION);
-
- //Setup mocks
- when(computeService.getContext().unwrapApi(AzureComputeApi.class)).thenReturn(azureComputeApi);
- when(azureComputeApi.getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME)).thenReturn(subnetApi);
- when(subnetApi.get(TEST_SUBNET_NAME)).thenReturn(null).thenReturn(subnet); //null first time, subnet next
- when(subnet.id()).thenReturn(TEST_SUBNET_ID);
-
- when(azureComputeApi.getResourceGroupApi()).thenReturn(resourceGroupApi);
- when(resourceGroupApi.get(TEST_RESOURCE_GROUP)).thenReturn(null);
-
- when(azureComputeApi.getVirtualNetworkApi(TEST_RESOURCE_GROUP)).thenReturn(virtualNetworkApi);
-
-
- //Test
- DefaultAzureArmNetworkCreator.createDefaultNetworkAndAddToTemplateOptionsIfRequired(computeService, configBag);
-
- //verify
- verify(subnetApi, times(2)).get(TEST_SUBNET_NAME);
- verify(subnet).id();
- verify(azureComputeApi, times(2)).getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME);
-
- verify(azureComputeApi, times(2)).getResourceGroupApi();
- verify(resourceGroupApi).get(TEST_RESOURCE_GROUP);
- verify(azureComputeApi).getVirtualNetworkApi(TEST_RESOURCE_GROUP);
-
- Map<String, Object> templateOptions = configBag.get(TEMPLATE_OPTIONS);
- Map<String, Object> ipOptions = (Map<String, Object>)templateOptions.get("ipOptions");
- assertEquals(ipOptions.get("subnet"), TEST_SUBNET_ID);
- assertEquals(ipOptions.get("allocateNewPublicIp"), true);
+ public void testVanillaWhereNoResourceGroup() throws Exception {
+ runVanilla(ImmutableMap.of(), false);
}
-
+
@Test
- public void testVanillaWhereTemplateOptionsAlreadySpecified() {
+ public void testVanillaWhereExistingResourceGroup() throws Exception {
+ runVanilla(ImmutableMap.of(), true);
+ }
+
+ @Test
+ public void testVanillaWhereTemplateOptionsAlreadySpecified() throws Exception {
+ ImmutableMap<?, ?> additionalConfig = ImmutableMap.of(TEMPLATE_OPTIONS, ImmutableMap.of("unrelated-key", "unrelated-value"));
+ ConfigBag result = runVanilla(additionalConfig, false);
+ Map<String, ?> templateOptions = result.get(TEMPLATE_OPTIONS);
+ assertEquals(templateOptions.get("unrelated-key"), "unrelated-value");
+ }
+
+ protected ConfigBag runVanilla(Map<?, ?> additionalConfig, boolean resoureGroupExists) throws Exception {
--- End diff --
I can see why you've created a generic vanilla test here (looks like it cuts down on code repetition), but I think that it introduces a couple of problems:
*) Makes the tests more difficult to understand (the runVanilla method itself is longer than an individual test used to be due to the necessity of adding if blocks to cope with different resource group cases)
*) Makes the tests more difficult to maintain (if the mock calls change for one of the tests, you have to modify the generic test and understand what all the other tests that rely on it expect it to do)
The tests themselves were pretty short before and as such I don't see a justification for this change. I would suggest reverting this to repeating the code per test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
Squashed into single commit; merging as soon as jenkins build finishes again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
p.s. I'll squash together my two commits before we merge this! So please ping me with any other suggested changes, or if you think it's good to merge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/742#discussion_r123495340
--- Diff: locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreatorTest.java ---
@@ -204,7 +199,7 @@ public void testNetworkInTemplate() {
}
@Test
- public void testIpOptionsInTemplate() {
+ public void testIpOptionsInTemplate() throws Exception {
--- End diff --
My personal option is that it's always a good idea for test methods to declare `throws Exception`. We're not writing the tests to assert that the signature of the method throws only specific checked exceptions (or no checked exception). Therefore not including the `throws Exception` just makes it more likely that subsequent refactoring might cause tests to fail to compile (which is annoying, as it might well have nothing to do with the tests it impacts). That's my opinion from past experience. In Brooklyn, it matters a lot less because we use unchecked exceptions a lot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
lgtm thanks @aledsage
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
Merged.
@neykov (cc @Graeme-Miller) thanks for pointer to your comments in (the already merged) #739. We should address those separately, in a new PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/742
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/742#discussion_r123482786
--- Diff: locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreatorTest.java ---
@@ -204,7 +199,7 @@ public void testNetworkInTemplate() {
}
@Test
- public void testIpOptionsInTemplate() {
+ public void testIpOptionsInTemplate() throws Exception {
--- End diff --
do these throw exceptions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #742: DefaultAzureArmNetworkCreator improvement...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/742#discussion_r123496290
--- Diff: locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/creator/DefaultAzureArmNetworkCreatorTest.java ---
@@ -90,62 +97,44 @@ public void testPreExisting() {
verify(azureComputeApi).getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME);
Map<String, Object> templateOptions = configBag.get(TEMPLATE_OPTIONS);
- Map<String, Object> ipOptions = (Map<String, Object>)templateOptions.get("ipOptions");
+ Map<?, ?> ipOptions = (Map<?, ?>)templateOptions.get("ipOptions");
assertEquals(ipOptions.get("subnet"), TEST_SUBNET_ID);
assertEquals(ipOptions.get("allocateNewPublicIp"), true);
}
@Test
- public void testVanilla() {
- //Setup config bag
- ConfigBag configBag = ConfigBag.newInstance();
- configBag.put(CLOUD_REGION_ID, TEST_LOCATION);
-
- //Setup mocks
- when(computeService.getContext().unwrapApi(AzureComputeApi.class)).thenReturn(azureComputeApi);
- when(azureComputeApi.getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME)).thenReturn(subnetApi);
- when(subnetApi.get(TEST_SUBNET_NAME)).thenReturn(null).thenReturn(subnet); //null first time, subnet next
- when(subnet.id()).thenReturn(TEST_SUBNET_ID);
-
- when(azureComputeApi.getResourceGroupApi()).thenReturn(resourceGroupApi);
- when(resourceGroupApi.get(TEST_RESOURCE_GROUP)).thenReturn(null);
-
- when(azureComputeApi.getVirtualNetworkApi(TEST_RESOURCE_GROUP)).thenReturn(virtualNetworkApi);
-
-
- //Test
- DefaultAzureArmNetworkCreator.createDefaultNetworkAndAddToTemplateOptionsIfRequired(computeService, configBag);
-
- //verify
- verify(subnetApi, times(2)).get(TEST_SUBNET_NAME);
- verify(subnet).id();
- verify(azureComputeApi, times(2)).getSubnetApi(TEST_RESOURCE_GROUP, TEST_NETWORK_NAME);
-
- verify(azureComputeApi, times(2)).getResourceGroupApi();
- verify(resourceGroupApi).get(TEST_RESOURCE_GROUP);
- verify(azureComputeApi).getVirtualNetworkApi(TEST_RESOURCE_GROUP);
-
- Map<String, Object> templateOptions = configBag.get(TEMPLATE_OPTIONS);
- Map<String, Object> ipOptions = (Map<String, Object>)templateOptions.get("ipOptions");
- assertEquals(ipOptions.get("subnet"), TEST_SUBNET_ID);
- assertEquals(ipOptions.get("allocateNewPublicIp"), true);
+ public void testVanillaWhereNoResourceGroup() throws Exception {
+ runVanilla(ImmutableMap.of(), false);
}
-
+
@Test
- public void testVanillaWhereTemplateOptionsAlreadySpecified() {
+ public void testVanillaWhereExistingResourceGroup() throws Exception {
+ runVanilla(ImmutableMap.of(), true);
+ }
+
+ @Test
+ public void testVanillaWhereTemplateOptionsAlreadySpecified() throws Exception {
+ ImmutableMap<?, ?> additionalConfig = ImmutableMap.of(TEMPLATE_OPTIONS, ImmutableMap.of("unrelated-key", "unrelated-value"));
+ ConfigBag result = runVanilla(additionalConfig, false);
+ Map<String, ?> templateOptions = result.get(TEMPLATE_OPTIONS);
+ assertEquals(templateOptions.get("unrelated-key"), "unrelated-value");
+ }
+
+ protected ConfigBag runVanilla(Map<?, ?> additionalConfig, boolean resoureGroupExists) throws Exception {
--- End diff --
When I first wrote the `runVanilla` method, it was exactly the same length as your existing test method - everything was duplicated in the two methods, with the only difference being one line at the start (to set up config) and one line at the end (to assert the config was still there). The tests are logically identical: the behaviour should be the same if you pass extra templateOptions or not. Therefore having a `runVanilla(Map<?,?> additoinalConfig)` makes sense I think.
The more controversial thing is then when adding `boolean resourceGroupExists` (which was previously untested). I could have duplicated the method - no strong feelings.
I think we can make our tests less brittle by being more relaxed about what we assert. For example `verify(subnetApi, times(2)).get(TEST_SUBNET_NAME)` - do we really care that it's exactly twice, or do we just care that it's `atLeastOnce()`?
I'll split it into two separate test methods, with duplication, as you suggest.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #742: DefaultAzureArmNetworkCreator improvements
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/742
@Graeme-Miller incorporated review comments. Note that I've deleted some of the `verify` assertions. For example, I don't think it's worth calling `verify(azureComputeApi).getVirtualNetworkApi(TEST_RESOURCE_GROUP)` if we subsequently do `verify(virtualNetworkApi)` because the only way to get the `virtualNetworkApi` instance was to call the former method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---