Adding tests for Service Fabric commands and fixing some arugment parsing logic #3424

samedder · 2017-05-23T01:19:37Z

New changes for Service Fabric:

Unit tests for components related to argument validation for custom commands in Service Fabric module.
Several fixes for commands that were previously too restrictive when validating arguments
VCR Recordings for some commands that do not fall into the previous unit testing category
Refactor SF configuration into it's own class, for mock-ability

codecov-io · 2017-05-23T01:31:47Z

Codecov Report

Merging #3424 into master will increase coverage by 0.49%.
The diff coverage is 52.05%.

@@            Coverage Diff             @@
##           master    #3424      +/-   ##
==========================================
+ Coverage   71.62%   72.12%   +0.49%     
==========================================
  Files         420      422       +2     
  Lines       26209    26289      +80     
  Branches     3968     3977       +9     
==========================================
+ Hits        18773    18961     +188     
+ Misses       6258     6112     -146     
- Partials     1178     1216      +38

Impacted Files	Coverage Δ
...zure-cli-sf/azure/cli/command_modules/sf/config.py	`33.33% <33.33%> (ø)`
...zure-cli-sf/azure/cli/command_modules/sf/custom.py	`40.53% <52.84%> (+30.73%)`	⬆️
...re-cli-sf/azure/cli/command_modules/sf/_factory.py	`88.23% <84.61%> (+82.67%)`	⬆️
...ice/azure/cli/command_modules/appservice/custom.py	`73.3% <0%> (-0.37%)`	⬇️
...s/azure-cli-interactive/azclishell/az_completer.py	`59.9% <0%> (-0.19%)`	⬇️
...yvault/azure/cli/command_modules/keyvault/_help.py	`100% <0%> (ø)`	⬆️
...li-lab/azure/cli/command_modules/lab/validators.py	`34.11% <0%> (ø)`	⬆️
...i-cloud/azure/cli/command_modules/cloud/_params.py	`93.33% <0%> (ø)`	⬆️
...cli-find/azure/cli/command_modules/find/_params.py	`100% <0%> (ø)`	⬆️
...skhelp/azure/cli/command_modules/taskhelp/_help.py	`100% <0%> (ø)`	⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14cd4ed...9aa5725. Read the comment docs.

troydai

Looks good besides a few minor issues. Please rebase your change to the latest master. We've updated pylint.

troydai · 2017-05-31T15:56:06Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/config.py

+        if using_ca == "True":
+            return self.az_config.get("servicefabric", "ca_path", fallback=None)
+        else:
+            return None


Per Pylint 1.7.1

if using_ca == "True": return self.az_config.get("servicefabric", "ca_path", fallback=None) return None

troydai · 2017-05-31T15:57:47Z

src/command_modules/azure-cli-sf/setup.py

@@ -34,7 +34,8 @@
 DEPENDENCIES = [
    'azure-servicefabric==5.6.130',
    'azure-cli-core',
-    'adal>=0.4.3'
+    'adal>=0.4.3',
+    'mock==2.0.0'


mock>=2.0.0 do not fix version in setup. We want the user to be able to patch a dependency. And why do we need mock in the production code? It is a testing utility.

troydai · 2017-05-31T16:01:02Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/custom.py

@@ -3,6 +3,7 @@
 # Licensed under the MIT License. See License.txt in the project root for license information.
 # --------------------------------------------------------------------------------------------
 # pylint: disable=too-many-lines
+# pylint: disable=too-many-locals


Can you mark this disable statement to the function where it is needed? It is not encouraged to disable a pylint rule for the scope of the entire file.

troydai · 2017-05-31T16:02:08Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/custom.py

+
+
+def parse_app_metrics(formatted_metrics):
+    from azure.servicefabric.models.application_metric_description import (


The parentheses don't seem to be necessary.

troydai · 2017-05-31T16:05:07Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/custom.py

+        NamedPartitionSchemeDescription
+    )
+    # pylint: disable=line-too-long
+    from azure.servicefabric.models.singleton_partition_scheme_description import (  # noqa: justification, no way to shorten


# noqa is not needed.

troydai · 2017-05-31T16:10:29Z

Please also take into the account that the line limitation is 120 instead if 100 now.

troydai · 2017-06-01T14:58:58Z

ping @samedder

samedder · 2017-06-01T15:49:06Z

@troydai. On vacation currently, can this wait till next week? I agree with all your comments.

troydai · 2017-06-01T15:59:33Z

@samedder sure. no hurry. enjoy your vacation.

troydai · 2017-06-07T02:35:35Z

@samedder please update the pull request when you have time.

samedder · 2017-06-07T04:36:19Z

@troydai will get to this tommorrow

troydai

Looks good to me overall. Please the only comments about dependency version and this is good to go.

troydai · 2017-06-08T16:29:37Z

src/command_modules/azure-cli-sf/setup.py

@@ -35,7 +35,7 @@
    'azure-servicefabric==5.6.130',


In the setup, the version of the dependencies shouldn't be pinned. Otherwise it will prevents user from applying patch.

We want to specifically pin this because we don't want to users to auto-upgrade at this time. I know we have pending work to move into our own CLI, so for the time being I'd just like to leave this as-is, since this version won't change until then anyways. Lets discuss over email what's the right thing to do with others.

troydai · 2017-06-08T17:20:28Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/_factory.py

-    endpoint = sf_get_connection_endpoint()
+def cf_sf_client(_):
+    sf_config = SfConfigParser()
+    endpoint = sf_config.connection_endpoint()
    if endpoint is None:


if not endpoint:

Unless endpoint == '' as any special meaning and should be accepted.

troydai · 2017-06-08T17:22:00Z

src/command_modules/azure-cli-sf/azure/cli/command_modules/sf/config.py

+
+    def ca_cert_info(self):
+        using_ca = self.az_config.get("servicefabric", "use_ca", fallback="False")
+        if using_ca == "True":


Just want to make sure that you understand the string comparison is case-sensitive.

Yes, but since we are the only ones who set these strings we expect with the casing that we set them.

samedder · 2017-06-08T19:18:47Z

@troydai I think we may have a transient fault in the CI:

==== Test module dls ====
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

troydai · 2017-06-08T19:54:43Z

I restart the build. Let's see if the issue persist.

samedder added 16 commits May 11, 2017 12:59

initial commit

923cce7

Initial tests

cb5acb3

Adding first round of select tests

12324d6

Adding app parse tests

ef7499a

Adding test logic for parsing application metrics

e8661f7

Adding service type health policy tests

e1dc210

Adding some service tests

581827e

Adding more service tests

7558ee5

Adding last service and node tests

2ac357a

Adding mock for client calls

c47e33e

Migrating to class for config parsing for mocking

4ee5c6f

Adding recordings and new tests

9f3cfd3

Fixing some tests that were previously broken

d7d6800

Adding disable for too many locals

55259b7

Merge branch 'master' into sf_tests

d205a7f

Updating history

fa31c4b

msftclas added the cla-not-required label May 23, 2017

troydai suggested changes May 31, 2017

View reviewed changes

samedder added 4 commits June 7, 2017 16:00

Merge branch 'master' into sf_tests

521b2ba

Fixing PR comments

3e3be90

Fixing tests and recordings

a81b2bf

Adding new recordings

729b24f

troydai approved these changes Jun 8, 2017

View reviewed changes

Fixing string truth-ness

9aa5725

troydai merged commit 2b563f4 into Azure:master Jun 8, 2017

haroldrandom added the cla-not-required label Oct 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding tests for Service Fabric commands and fixing some arugment parsing logic #3424

Adding tests for Service Fabric commands and fixing some arugment parsing logic #3424

samedder commented May 23, 2017

codecov-io commented May 23, 2017 •

edited

Loading

troydai left a comment

troydai May 31, 2017

troydai May 31, 2017

troydai May 31, 2017

troydai May 31, 2017

troydai May 31, 2017

troydai commented May 31, 2017

troydai commented Jun 1, 2017

samedder commented Jun 1, 2017

troydai commented Jun 1, 2017

troydai commented Jun 7, 2017

samedder commented Jun 7, 2017

troydai left a comment

troydai Jun 8, 2017

samedder Jun 8, 2017

troydai Jun 8, 2017

troydai Jun 8, 2017

samedder Jun 8, 2017

samedder commented Jun 8, 2017

troydai commented Jun 8, 2017



		def parse_app_metrics(formatted_metrics):
		from azure.servicefabric.models.application_metric_description import (

Adding tests for Service Fabric commands and fixing some arugment parsing logic #3424

Adding tests for Service Fabric commands and fixing some arugment parsing logic #3424

Conversation

samedder commented May 23, 2017

codecov-io commented May 23, 2017 • edited Loading

Codecov Report

troydai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troydai commented May 31, 2017

troydai commented Jun 1, 2017

samedder commented Jun 1, 2017

troydai commented Jun 1, 2017

troydai commented Jun 7, 2017

samedder commented Jun 7, 2017

troydai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samedder commented Jun 8, 2017

troydai commented Jun 8, 2017

codecov-io commented May 23, 2017 •

edited

Loading