Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 'azdev test' automation and storage stack tests #7182

Merged
merged 4 commits into from Aug 30, 2018
Merged

Conversation

williexu
Copy link
Contributor

@williexu williexu commented Aug 30, 2018


Fixes: #7179
Moved storage_test_util into tests from within profiles.

@williexu williexu requested a review from troydai August 30, 2018 00:32
Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider an alternative that doesn't add maintenance burden. With this approach, some more robust error handling is needed.

'2017-03-09-profile': 'profile_2017_03_09',
'2018-03-01-hybrid': 'hybrid_2018_03_01',
'latest': 'latest'
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just one more thing to maintain. These follow a consistent pattern, why not just special-case latest and for the others, do the dash to underscore replacement and move the end word to the beginning.

@@ -177,7 +181,7 @@ def discover_tests(args):

CORE_EXCLUSIONS = ['command_modules', '__main__', 'testsdk']

profile = args.profile.replace('-', '_')
profile_namespace = PROFILE_TO_NAMESPACE[args.profile]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will crash when a new profile is added that doesn't get added to the PROFILE_TO_NAMESPACE dictionary (which is pretty likely). If you stick with this, at least catch the error and throw a friendly error to direct the dev to add the profile to the dictionary.

Another alternative would be to have this as part of the standard "shared.py" file in azure.cli.core.profiles. This makes it less likely a profile will be added without this.

@williexu
Copy link
Contributor Author

@tjprescott made the change

@williexu williexu merged commit b4f4a8b into Azure:dev Aug 30, 2018
@williexu williexu deleted the azdev branch October 24, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants