fix(citest): Dont perform superfluous platform initializations #1529
+788
−447
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This CL refactors SpinnakerTestScenario so that bindings requiring platform interrogation are not performed until they are needed. This allows tests to be decoupled from platforms they are not using. This also refactors the SpinnakerTestScenario so that the platform specific code is factored out into an encapsulated support module. This should make it easier to add and maintain new platforms as well as simplify what's going on.
lwander
reviewed
Apr 1, 2017
| builder.add_argument( | ||
| '--test_aws_vpc_id', | ||
| default=defaults.get('TEST_AWS_VPC_ID', None), | ||
| help='Default AWS VpcId to use when creating test resources.') |
There was a problem hiding this comment.
Why not "VPC ID" rather than "VpcId"?
There was a problem hiding this comment.
VpcId is the identifier in the AWS schemas. Not that it matters.
|
I'm missing something - where is the loading from lazy bindings defined? |
|
Not sure what you mean by where loading is defined.
There's only one place it is currently used, which is in the AWS support
around here
41e6aa3#diff-7d5658c9672146f06d4d0722fc4675a0R123
The mechanism is in citest, the PR defined. It adds the injected function
map into the "key" search path.
…On Sat, Apr 1, 2017 at 2:51 PM, Lars Wander ***@***.***> wrote:
I'm missing something - where is the loading from lazy bindings defined?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1529 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEKkGF04pP3ixSFCYea0y7_ERmijMJ4Sks5rrpy6gaJpZM4MwkBQ>
.
|
|
What I meant was I see you call |
|
It's in the other cited PR
google/citest#252
…On Sun, Apr 2, 2017 at 5:19 PM, Lars Wander ***@***.***> wrote:
What I meant was I see you call bindings.add_lazy_initializer, and I see
you read properties from the bindings that were added with that method, but
I didn't see where the logic for that lives.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1529 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEKkGOUkPV2vFglqnZMaKEXZ_Ead7QjNks5rsBDwgaJpZM4MwkBQ>
.
|
|
@ewiseblatt Okay! I will test and will let you know. |
|
I submitted the citest change so this is all you need to patch in.
…On Mon, Apr 3, 2017 at 9:18 AM, Roshan ***@***.***> wrote:
@ewiseblatt <https://github.com/ewiseblatt> Okay! I will test and will
let you know.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1529 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEKkGMcbFwJp6RGC03wklbrItENPYBppks5rsPG7gaJpZM4MwkBQ>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@rguthriemsft When the dependent PR goes in, you should take things you may be adding into spinnaker_test_scenario.py and instead add them to an azure_support.py so the only change to spinnaker_test_scenario is adding AzureScenarioSupport to the PLATFORM_SUPPORT_CLASSES. This PR should fix the other issues made me aware of so your workarounds should not be needed anymore.
@Roshan2017 I migrated all the OpenStack changes, but I did not test them because I still havent gotten around to reinstalling openstack. Could you test this for me?
@jtk54 or @lwander Please review.
This CL refactors SpinnakerTestScenario so that bindings requiring
platform interrogation are not performed until they are needed.
This allows tests to be decoupled from platforms they are not using.
This also refactors the SpinnakerTestScenario so that the platform
specific code is factored out into an encapsulated support module.
This should make it easier to add and maintain platforms, as
well as simplify what's going on.
Most of this PR is moving existing code from spinnaker_scenario_test.py into individual _support.py modules. The interface has a method for initializing the argparser (used before the class is constructed), initializing the bindings (used on the instance after constructed) and creating the observer for that platform. The appropriate code blocks were moved from spinnaker_testing without any major change except as follows:
The observers are not constructed until the property is referenced. So if the observer is never referenced, it will never be instantiated. Presumably if one is not needed it will not be referenced.
Binding values derived from interrogating the platform if not otherwise available are added using lazy initializers so that they will not incur the work if they are never needed. This also decouples the platform initialization from tests that do not need them.
I added a SPINNAKER__ENABLED binding to easily check if a platform is enabled or not rather than merely seeing if there is an account configured for it. This is added in the base support class.
This PR depends on google/citest#252 which is not yet approved.