DefaultAzureArmNetworkCreator improvements

[aledsage]

• 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.

[Graeme-Miller]

Just one comment to address

[Graeme-Miller]

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.

[aledsage]

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.

[Graeme-Miller]

do these throw exceptions?

[aledsage]

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.

[aledsage]

@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.

[aledsage]

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.

[Graeme-Miller]

lgtm thanks @aledsage

[aledsage]

Squashed into single commit; merging as soon as jenkins build finishes again.

[neykov]

LGTM, also see the comments I just added to #739

[aledsage]

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.