Skip to content

Commit

Permalink
Cleanup case module for increased clarity.
Browse files Browse the repository at this point in the history
* Functions moved to utils module
* http request moved to own method
* redundant data assignment of response and output rationalized
  • Loading branch information
cdent committed Mar 8, 2015
1 parent 19f3f29 commit 753ab77
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 49 deletions.
79 changes: 30 additions & 49 deletions gabbi/case.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from six.moves.urllib import parse as urlparse
from testtools import testcase

from gabbi import utils

REPLACERS = [
'SCHEME',
Expand Down Expand Up @@ -125,9 +126,12 @@ def replace_template(self, message):

return message

def _assert_response(self, response, test):
def _assert_response(self):
"""Compare the response with expected data."""

test = self.test_data
response = self.response

# Never accept a 500

This comment has been minimized.

Copy link
@FND

FND Oct 18, 2015

Collaborator

I understand the intent here, but it seems like unnecessary special-casing: Do we really want to prohibit users from explicitly expecting a 500? After all, such an expectation might be temporary, e.g. as part of TDD or while cleaning up an existing implementation.

This comment has been minimized.

Copy link
@cdent

cdent Oct 18, 2015

Author Owner

I suspect this got in there during a time when I was very grumpy with the openstack environment for thinking that a 500 (or any untrapped exception in general) was an okay way of handling transient errors such as a database being unavailable, so I stuck this in there as a hard reminder.

I do think that expecting a 500 is a really bad habit to allow but I'm willing to get rid of this chunk if it is a pain for people.

This comment has been minimized.

Copy link
@FND

FND Oct 18, 2015

Collaborator

if it is a pain for people

I don't suppose it is (yet anyway, though that will change once gabbi has taken over the world) - it was just something that struck me as somewhat out of place while reading the code, i.e. from an implementor's perspective.

This comment has been minimized.

Copy link
@FND

FND Oct 19, 2015

Collaborator

So it turns out that this causes pain after all: It circumvents the truncation that otherwise happens in assert_in_or_print_output, thus risking overwhelming the user.

So as discussed elsewhere, I'll remove it.

if response['status'] == '500':
raise ServerError(self.output)
Expand Down Expand Up @@ -244,6 +248,28 @@ def _response_replace(self, message):
return re.sub(r"\$RESPONSE\['([^']+)'\]",
self._json_replacer, message)

def _run_request(self, url, method, headers, body):
"""Run the http request and decode output."""

response, content = self.http.request(
url,
method=method,
headers=headers,
body=body
)

# Set headers and location attributes for follow on requests
self.response = response
if 'location' in response:
self.location = response['location']

# Decode and store response
decoded_output = utils.decode_content(response, content)
if (decoded_output and
'application/json' in response.get('content-type', '')):
self.json_data = json.loads(decoded_output)
self.output = decoded_output

def _run_test(self):
"""Make an HTTP request and compare the response with expectations."""
test = self.test_data
Expand All @@ -267,27 +293,8 @@ def _run_test(self):
if test['redirects']:
self.http.follow_redirects = True

# Make the actual request.
response, content = self.http.request(
full_url,
method=method,
headers=headers,
body=body
)

# Set headers and location attributes for follow on requests
self.response = response
if 'location' in response:
self.location = response['location']

# Decode and store response before anything else
decoded_output = _decode_content(response, content)
if (decoded_output and
'application/json' in response.get('content-type', '')):
self.json_data = json.loads(decoded_output)
self.output = decoded_output

self._assert_response(response, test)
self._run_request(full_url, method, headers, body)
self._assert_response()

def _scheme_replace(self, message):
"""Replace $SCHEME with the current protocol."""
Expand All @@ -301,7 +308,7 @@ def _test_data_to_string(self, data, content_type):
if isinstance(data, str):
if data.startswith('<@'):
info = self._load_data_file(data.replace('<@', '', 1))
if _not_binary(content_type):
if utils.not_binary(content_type):
try:
info = str(info, 'UTF-8')
except TypeError:
Expand Down Expand Up @@ -354,32 +361,6 @@ def _test_status(self, expected_status, observed_status):
self.assertIn(observed_status, statii)


def _decode_content(response, content):
"""Decode content to a proper string."""
content_type = response.get('content-type',
'application/binary').lower()
if ';' in content_type:
content_type, charset = (attr.strip() for attr in
content_type.split(';'))
charset = charset.split('=')[1].strip()
else:
charset = 'utf-8'

if _not_binary(content_type):
return content.decode(charset)
else:
return content


def _not_binary(content_type):
"""Decide if something is content we'd like to treat as a string."""
return (content_type.startswith('text/') or
content_type.endswith('+xml') or
content_type.endswith('+json') or
content_type == 'application/javascript' or
content_type == 'application/json')


class ServerError(Exception):
"""A catchall ServerError."""
pass
42 changes: 42 additions & 0 deletions gabbi/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Copyright 2014, 2015 Red Hat
#
# Authors: Chris Dent <chdent@redhat.com>
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is 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.
"""Utility functions grab bag."""


def decode_content(response, content):
"""Decode content to a proper string."""
content_type = response.get('content-type',
'application/binary').lower()
if ';' in content_type:
content_type, charset = (attr.strip() for attr in
content_type.split(';'))
charset = charset.split('=')[1].strip()
else:
charset = 'utf-8'

if not_binary(content_type):
return content.decode(charset)
else:
return content


def not_binary(content_type):
"""Decide if something is content we'd like to treat as a string."""
return (content_type.startswith('text/') or
content_type.endswith('+xml') or
content_type.endswith('+json') or
content_type == 'application/javascript' or
content_type == 'application/json')

0 comments on commit 753ab77

Please sign in to comment.