Repository: boto/botocore
Pull request #1032 (2016-09-15 19:42:56)

Improve functional test for waiter config validation
This improves the waiter config validation to check a few additional cases, primarily related to JMESPath including:

* Verify the JMESPath expression matches something in the output
* Verify the JMESPath expression returns list types for `pathAll` and `pathAny`
* Verify the operation referenced actually produces output.

In order to do this, I made one change to the `ArgumentGenerator` utility class.  In order to rely on the "truth" value of the jmespath search result, I didn't want empty strings being generated.  To account for this, I added the ability for the placeholder strings to optionally use the member names for the values.

As part of this change, I had to update one of the waiter configs as it was now failing the tests:

```
AssertionError: Attempted to use 'pathAll' matcher in waiter 'KeyPairExists' with \
 non list result in JMESPath expression: length(KeyPairs[].KeyName) > `0`
```

I also moved around when we create a waiter so we now get better error messages from pre-existing validation.  Here's what some of the error messages now look like:

```
AssertionError: JMESPath expression did not match anything for \
  waiter 'ConversionTaskCancelled': ConversionTasks[].WillNeverMatch

AssertionError: Could not create waiter 'bundle_task_complete': \
  Invalid jmespath expression: Incomplete expression: "BadJMESPath("

AssertionError: Could not create waiter 'nat_gateway_available': \
  Error processing waiter config: Unknown acceptor: pathUnknown

AssertionError: Waiter 'UserExists' has JMESPath expression with \
  no output shape: OperationModel(name=DeleteUser)
```

cc @kyleknap @JordonPhillips 


Patch:
diff --git a/botocore/data/ec2/2016-04-01/waiters-2.json b/botocore/data/ec2/2016-04-01/waiters-2.json
index 8d46efb5c..aa36a0440 100644
--- a/botocore/data/ec2/2016-04-01/waiters-2.json
+++ b/botocore/data/ec2/2016-04-01/waiters-2.json
@@ -301,7 +301,7 @@
       "acceptors": [
         {
           "expected": true,
-          "matcher": "pathAll",
+          "matcher": "path",
           "state": "success",
           "argument": "length(KeyPairs[].KeyName) > `0`"
         },
diff --git a/botocore/utils.py b/botocore/utils.py
index a9cbc2a03..fa7dde9c9 100644
--- a/botocore/utils.py
+++ b/botocore/utils.py
@@ -503,12 +503,13 @@ class ArgumentGenerator(object):
     """Generate sample input based on a shape model.
 
     This class contains a ``generate_skeleton`` method that will take
-    an input shape (created from ``botocore.model``) and generate
-    a sample dictionary corresponding to the input shape.
+    an input/output shape (created from ``botocore.model``) and generate
+    a sample dictionary corresponding to the input/output shape.
 
-    The specific values used are place holder values. For strings an
-    empty string is used, for numbers 0 or 0.0 is used.  The intended
-    usage of this class is to generate the *shape* of the input structure.
+    The specific values used are place holder values. For strings either an
+    empty string or the member name can be used, for numbers 0 or 0.0 is used.
+    The intended usage of this class is to generate the *shape* of the input
+    structure.
 
     This can be useful for operations that have complex input shapes.
     This allows a user to just fill in the necessary data instead of
@@ -524,8 +525,9 @@ class ArgumentGenerator(object):
         print("Sample input for dynamodb.CreateTable: %s" % sample_input)
 
     """
-    def __init__(self):
-        pass
+    def __init__(self, use_member_names=False):
+        self._use_member_names = use_member_names
+        self._max_stack_depth = 1
 
     def generate_skeleton(self, shape):
         """Generate a sample input.
@@ -540,7 +542,7 @@ class ArgumentGenerator(object):
         stack = []
         return self._generate_skeleton(shape, stack)
 
-    def _generate_skeleton(self, shape, stack):
+    def _generate_skeleton(self, shape, stack, name=''):
         stack.append(shape.name)
         try:
             if shape.type_name == 'structure':
@@ -550,6 +552,8 @@ class ArgumentGenerator(object):
             elif shape.type_name == 'map':
                 return self._generate_type_map(shape, stack)
             elif shape.type_name == 'string':
+                if self._use_member_names:
+                    return name
                 return ''
             elif shape.type_name in ['integer', 'long']:
                 return 0
@@ -565,8 +569,8 @@ class ArgumentGenerator(object):
             return {}
         skeleton = OrderedDict()
         for member_name, member_shape in shape.members.items():
-            skeleton[member_name] = self._generate_skeleton(member_shape,
-                                                            stack)
+            skeleton[member_name] = self._generate_skeleton(
+                member_shape, stack, name=member_name)
         return skeleton
 
     def _generate_type_list(self, shape, stack):


Test patch:
diff --git a/tests/functional/test_waiter_config.py b/tests/functional/test_waiter_config.py
index a32fb5631..2aa35de81 100644
--- a/tests/functional/test_waiter_config.py
+++ b/tests/functional/test_waiter_config.py
@@ -10,9 +10,10 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
 # ANY KIND, either express or implied. See the License for the specific
 # language governing permissions and limitations under the License.
-from nose.tools import assert_equals
+import jmespath
 
 import botocore.session
+from botocore.utils import ArgumentGenerator
 
 
 def test_lint_waiter_configs():
@@ -21,11 +22,23 @@ def test_lint_waiter_configs():
         client = session.create_client(service_name, 'us-east-1')
         service_model = client.meta.service_model
         for waiter_name in client.waiter_names:
-            waiter = client.get_waiter(waiter_name)
-            yield _lint_single_waiter, waiter, service_model
+            yield _lint_single_waiter, client, waiter_name, service_model
 
 
-def _lint_single_waiter(waiter, service_model):
+def _lint_single_waiter(client, waiter_name, service_model):
+    try:
+        waiter = client.get_waiter(waiter_name)
+        # The 'acceptors' property is dynamic and will create
+        # the acceptor configs when first accessed.  This is still
+        # considered a failure to construct the waiter which is
+        # why it's in this try/except block.
+        # This catches things like:
+        # * jmespath expression compiles
+        # * matcher has a known value
+        acceptors = waiter.config.acceptors
+    except Exception as e:
+        raise AssertionError("Could not create waiter '%s': %s"
+                             % (waiter_name, e))
     operation_name = waiter.config.operation
     # Needs to reference an existing operation name.
     if operation_name not in service_model.operation_names:
@@ -35,8 +48,38 @@ def _lint_single_waiter(waiter, service_model):
     if not waiter.config.acceptors:
         raise AssertionError("Waiter config must have at least "
                              "one acceptor state: %s" % waiter.name)
-    # Additional things to add:
-    # 1. Verify the error acceptors correspond to a 'code' in the model
-    # 2. Verify JMESPath expressions can resolve to something in
-    #    the response.
+    op_model = service_model.operation_model(operation_name)
+    for acceptor in acceptors:
+        _validate_acceptor(acceptor, op_model, waiter.name)
 
+
+def _validate_acceptor(acceptor, op_model, waiter_name):
+    if acceptor.matcher.startswith('path'):
+        expression = acceptor.argument
+        # The JMESPath expression should have the potential to match something
+        # in the response shape.
+        output_shape = op_model.output_shape
+        assert output_shape is not None, (
+            "Waiter '%s' has JMESPath expression with no output shape: %s"
+            % (waiter_name, op_model))
+        # We want to check if the JMESPath expression makes sense.
+        # To do this, we'll generate sample output and evaluate the
+        # JMESPath expression against the output.  We'll then
+        # check a few things about this returned search result.
+        search_result = _search_jmespath_expression(expression, op_model)
+        if not search_result:
+            raise AssertionError("JMESPath expression did not match "
+                                 "anything for waiter '%s': %s"
+                                 % (waiter_name, expression))
+        if acceptor.matcher in ['pathAll', 'pathAny']:
+            assert isinstance(search_result, list), \
+                    ("Attempted to use '%s' matcher in waiter '%s' "
+                     "with non list result in JMESPath expression: %s"
+                     % (acceptor.matcher, waiter_name, expression))
+
+
+def _search_jmespath_expression(expression, op_model):
+    arg_gen = ArgumentGenerator(use_member_names=True)
+    sample_output = arg_gen.generate_skeleton(op_model.output_shape)
+    search_result = jmespath.search(expression, sample_output)
+    return search_result
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index 33ace8c46..5d5cd852c 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -366,6 +366,23 @@ class TestArgumentGenerator(unittest.TestCase):
             }
         )
 
+    def test_will_use_member_names_for_string_values(self):
+        self.arg_generator = ArgumentGenerator(use_member_names=True)
+        self.assert_skeleton_from_model_is(
+            model={
+                'A': {'type': 'string'},
+                'B': {'type': 'integer'},
+                'C': {'type': 'float'},
+                'D': {'type': 'boolean'},
+            },
+            generated_skeleton={
+                'A': 'A',
+                'B': 0,
+                'C': 0.0,
+                'D': True,
+            }
+        )
+
     def test_generate_nested_structure(self):
         self.assert_skeleton_from_model_is(
             model={