Skip to content

Commit

Permalink
Ensure defaults update correctly
Browse files Browse the repository at this point in the history 
String default values were updating appropriately, but not lists or
dicts. A test_update method is added to handle these correctly.
The 'data' key is treated specially as it might be a string, a list
or a dict. Whatever form it is, we want to replace the default, not
merge.
  • Loading branch information
@cdent
cdent committed Apr 20, 2015
1 parent 29fd7c1 commit 7d0be42
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 4 deletions.
24 changes: 20 additions & 4 deletions gabbi/driver.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@
An entire directory of YAML files is a TestSuite of TestSuites.
"""

import copy
import glob
import inspect
import os
from unittest import suite
import uuid

import httplib2
import six
import yaml

from gabbi import case
Expand Down Expand Up @@ -104,6 +106,19 @@ def load_yaml(yaml_file):
return yaml.safe_load(source.read())


def test_update(orig_dict, new_dict):
"""Modify test in place to update with new data."""
for key, val in six.iteritems(new_dict):
if key == 'data':
orig_dict[key] = val
elif isinstance(val, dict):
orig_dict[key].update(val)
elif isinstance(val, list):
orig_dict[key] = orig_dict.get(key, []) + val
else:
orig_dict[key] = val


def test_suite_from_yaml(loader, test_base_name, test_yaml, test_directory,
host, port, fixture_module, intercept):
"""Generate a TestSuite from YAML data."""
Expand All @@ -114,8 +129,9 @@ def test_suite_from_yaml(loader, test_base_name, test_yaml, test_directory,

# Set defaults from BASE_TESTS then update those defaults
# with any defaults set in the YAML file.
base_test_data = dict(case.HTTPTestCase.base_test)
base_test_data.update(test_yaml.get('defaults', {}))
base_test_data = copy.deepcopy(case.HTTPTestCase.base_test)

This comment has been minimized.

Sign in to view

Copy link
@FND

FND Oct 11, 2015

Collaborator

HTTPTestCase merely uses copy.copy (i.e. shallow), so why use deepcopy here?

Also, it strikes me as slightly puzzling that HTTPTestCase uses a copy in the first place; that shouldn't make a difference? (I'm guessing the only reason BASE_TEST is a module-level variable there rather than a class variable is to increase its visibility.)

This comment has been minimized.

Sign in to view

Copy link
@cdent

cdent Oct 11, 2015

Author Owner

It's a combination of paranoia, premature extensibility planning and protection against metaclass magic.

Injecting a few deepcopy here and there makes sure we don't have any stray references around to confuse things so I tend to default to that. Take that as the primary reason.

The initial copy is is okay and correct because BASE_TEST is acknowledge as non-data carrying, but everything after it might (imagine a future where dependency injection puts in a subclass of HTTPTestCase that manipulates base_test in some fashion).

Complicating all of this is that though base_test is a class level variable in HTTPTestCase, each test case that TestMaker and TestBuilder conspire to make is actually a class of its own using HTTPTestCase as its initial type (this is the metaclass magic and is how you give a test a distinct name in a collection of classes, having distinct name is a big reason why dynamic class generation is happening[1]). Thus the 'klass' in TestMaker, if you get the str of that it is something like: <class 'gabbi.driver.test_driver_sample_two'>.

So: in the context of all that complexity it feels safe to use deepcopy here and for the class to do a copy to have its own. It probably introduces some small number of redundancies but I'm happy with that for the sake of the boundaries provided.

[1] Any why py.test confederates think unittest people are crazy.

This comment has been minimized.

Sign in to view

Copy link
@FND

FND Oct 11, 2015

Collaborator

That all makes sense - thanks for clarifying.
defaults = test_yaml.get('defaults', {})
test_update(base_test_data, defaults)

# Establish any fixture classes.
fixture_classes = []
Expand All @@ -126,8 +142,8 @@ def test_suite_from_yaml(loader, test_base_name, test_yaml, test_directory,
prior_test = None
base_test_key_set = set(case.HTTPTestCase.base_test.keys())
for test_datum in test_data:
test = dict(base_test_data)
test.update(test_datum)
test = copy.deepcopy(base_test_data)
test_update(test, test_datum)

if not test['name']:
raise AssertionError('Test name missing in a test in %s.'
Expand Down
2 changes: 2 additions & 0 deletions gabbi/gabbits_intercept/self.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

defaults:
url: /cow?alpha=1
request_headers:
x-random-header: ya

tests:
- name: get simple page
Expand Down

0 comments on commit 7d0be42

Please sign in to comment.