Skip to content

Commit

Permalink
Ensure the reason is never an empty string, but None in HTTP responses
Browse files Browse the repository at this point in the history
Otherwise, `aiohttp` (in tests) cannot parse the response with the trailing space in the status — `"HTTP/1.1 555 "` — since `"555 ".split()` returns only one element, and the whole string `"555 "` is used as the status with expectations of being a 3-digit numeric.

For some reason, the tests fail with this only in PyPy (all versions).

Signed-off-by: Sergey Vasilyev <nolar@nolar.info>
  • Loading branch information
nolar committed Oct 8, 2023
1 parent 538df59 commit 0941153
Show file tree
Hide file tree
Showing 13 changed files with 30 additions and 26 deletions.
8 changes: 4 additions & 4 deletions kopf/_kits/webhooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,13 +214,13 @@ async def _serve(
response = await fn(data, webhook=webhook, sslpeer=sslpeer, headers=headers)
return aiohttp.web.json_response(response)
except admission.AmbiguousResourceError as e:
raise aiohttp.web.HTTPConflict(reason=str(e))
raise aiohttp.web.HTTPConflict(reason=str(e) or None)
except admission.UnknownResourceError as e:
raise aiohttp.web.HTTPNotFound(reason=str(e))
raise aiohttp.web.HTTPNotFound(reason=str(e) or None)
except admission.WebhookError as e:
raise aiohttp.web.HTTPBadRequest(reason=str(e))
raise aiohttp.web.HTTPBadRequest(reason=str(e) or None)
except json.JSONDecodeError as e:
raise aiohttp.web.HTTPBadRequest(reason=str(e))
raise aiohttp.web.HTTPBadRequest(reason=str(e) or None)

@staticmethod
def _allocate_free_port() -> int:
Expand Down
2 changes: 1 addition & 1 deletion tests/apis/test_api_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ async def test_raw_requests_are_not_parsed(
async def test_server_errors_escalate(
resp_mocker, aresponses, hostname, method, settings, logger):

mock = resp_mocker(return_value=aiohttp.web.json_response({}, status=666))
mock = resp_mocker(return_value=aiohttp.web.json_response({}, status=666, reason='oops'))
aresponses.add(hostname, '/url', method, mock)
with pytest.raises(APIError) as err:
await request(method, '/url', settings=settings, logger=logger)
Expand Down
14 changes: 7 additions & 7 deletions tests/apis/test_error_retries.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ async def test_client_errors_escalate_without_retries(
caplog.set_level(0)

# side_effect instead of return_value -- to generate a new response on every call, not reuse it.
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=status))
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=status, reason='oops'))
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
Expand All @@ -60,7 +60,7 @@ async def test_server_errors_escalate_with_retries(
caplog.set_level(0)

# side_effect instead of return_value -- to generate a new response on every call, not reuse it.
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=status))
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=status, reason='oops'))
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
Expand Down Expand Up @@ -123,8 +123,8 @@ async def test_retried_until_succeeded(
caplog.set_level(0)

mock = resp_mocker(side_effect=[
aiohttp.web.json_response({}, status=505),
aiohttp.web.json_response({}, status=505),
aiohttp.web.json_response({}, status=505, reason='oops'),
aiohttp.web.json_response({}, status=505, reason='oops'),
aiohttp.web.json_response({}),
])
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
Expand Down Expand Up @@ -158,7 +158,7 @@ async def test_backoffs_as_lists(
caplog.set_level(0)

# side_effect instead of return_value -- to generate a new response on every call, not reuse it.
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500))
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500, reason='oops'))
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
Expand All @@ -178,7 +178,7 @@ async def test_backoffs_as_floats(
caplog.set_level(0)

# side_effect instead of return_value -- to generate a new response on every call, not reuse it.
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500))
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500, reason='oops'))
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts

Expand All @@ -200,7 +200,7 @@ def __iter__(self):
return iter([1, 2, 3])

# side_effect instead of return_value -- to generate a new response on every call, not reuse it.
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500))
mock = resp_mocker(side_effect=lambda: aiohttp.web.json_response({}, status=500, reason='oops'))
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
aresponses.add(hostname, '/url', 'get', mock) # repeat=N would copy the mock, lose all counts
Expand Down
2 changes: 1 addition & 1 deletion tests/k8s/test_creating.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ async def test_raises_api_errors(
resp_mocker, aresponses, hostname, settings, status, resource, namespace, logger,
cluster_resource, namespaced_resource):

post_mock = resp_mocker(return_value=aresponses.Response(status=status))
post_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
cluster_url = cluster_resource.get_url(namespace=None)
namespaced_url = namespaced_resource.get_url(namespace='ns')
aresponses.add(hostname, cluster_url, 'post', post_mock)
Expand Down
4 changes: 4 additions & 0 deletions tests/k8s/test_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ async def test_no_error_on_success(

resp = aresponses.Response(
status=status,
reason='oops',
headers={'Content-Type': 'application/json'},
text='{"kind": "Status", "code": "xxx", "message": "msg"}',
)
Expand Down Expand Up @@ -67,6 +68,7 @@ async def test_error_with_payload(

resp = aresponses.Response(
status=status,
reason='oops',
headers={'Content-Type': 'application/json'},
text='{"kind": "Status", "code": 123, "message": "msg", "details": {"a": "b"}}',
)
Expand All @@ -89,6 +91,7 @@ async def test_error_with_nonjson_payload(

resp = aresponses.Response(
status=status,
reason='oops',
headers={'Content-Type': 'application/json'},
text='unparsable json',
)
Expand All @@ -109,6 +112,7 @@ async def test_error_with_parseable_nonk8s_payload(

resp = aresponses.Response(
status=status,
reason='oops',
headers={'Content-Type': 'application/json'},
text='{"kind": "NonStatus", "code": "xxx", "message": "msg"}',
)
Expand Down
4 changes: 2 additions & 2 deletions tests/k8s/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ async def test_no_events_for_events(
async def test_api_errors_logged_but_suppressed(
resp_mocker, aresponses, hostname, settings, logger, assert_logs):

post_mock = resp_mocker(return_value=aresponses.Response(status=555))
post_mock = resp_mocker(return_value=aresponses.Response(status=555, reason='oops'))
aresponses.add(hostname, '/api/v1/namespaces/ns/events', 'post', post_mock)

obj = {'apiVersion': 'group/version',
Expand Down Expand Up @@ -161,7 +161,7 @@ async def test_message_is_cut_to_max_length(
async def test_headers_are_not_leaked(
resp_mocker, aresponses, hostname, settings, logger, assert_logs, status):

post_mock = resp_mocker(return_value=aresponses.Response(status=status))
post_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
aresponses.add(hostname, '/api/v1/namespaces/ns/events', 'post', post_mock)

obj = {'apiVersion': 'group/version',
Expand Down
2 changes: 1 addition & 1 deletion tests/k8s/test_list_objs.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ async def test_raises_direct_api_errors(
resp_mocker, aresponses, hostname, settings, logger, status, resource, namespace,
cluster_resource, namespaced_resource):

list_mock = resp_mocker(return_value=aresponses.Response(status=status))
list_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
cluster_url = cluster_resource.get_url(namespace=None)
namespaced_url = namespaced_resource.get_url(namespace='ns')
aresponses.add(hostname, cluster_url, 'get', list_mock)
Expand Down
4 changes: 2 additions & 2 deletions tests/k8s/test_patching.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ async def test_ignores_absent_objects(
resp_mocker, aresponses, hostname, settings, status, resource, namespace, logger,
cluster_resource, namespaced_resource):

patch_mock = resp_mocker(return_value=aresponses.Response(status=status))
patch_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
cluster_url = cluster_resource.get_url(namespace=None, name='name1')
namespaced_url = namespaced_resource.get_url(namespace='ns', name='name1')
aresponses.add(hostname, cluster_url, 'patch', patch_mock)
Expand All @@ -225,7 +225,7 @@ async def test_raises_api_errors(
resp_mocker, aresponses, hostname, settings, status, resource, namespace, logger,
cluster_resource, namespaced_resource):

patch_mock = resp_mocker(return_value=aresponses.Response(status=status))
patch_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
cluster_url = cluster_resource.get_url(namespace=None, name='name1')
namespaced_url = namespaced_resource.get_url(namespace='ns', name='name1')
aresponses.add(hostname, cluster_url, 'patch', patch_mock)
Expand Down
8 changes: 4 additions & 4 deletions tests/k8s/test_scanning.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ async def test_http404_returns_no_resources_from_old_apis(

core_mock = resp_mocker(return_value=aiohttp.web.json_response({'versions': ['v1']}))
apis_mock = resp_mocker(return_value=aiohttp.web.json_response({'groups': []}))
status_mock = resp_mocker(return_value=aresponses.Response(status=status))
status_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
aresponses.add(hostname, '/api', 'get', core_mock)
aresponses.add(hostname, '/apis', 'get', apis_mock)
aresponses.add(hostname, '/api/v1', 'get', status_mock)
Expand All @@ -259,7 +259,7 @@ async def test_http404_returns_no_resources_from_new_apis(
apis_mock = resp_mocker(return_value=aiohttp.web.json_response({'groups': [
{'name': 'g1', 'preferredVersion': {'version': ''}, 'versions': [{'version': 'g1v1'}]},
]}))
status_mock = resp_mocker(return_value=aresponses.Response(status=status))
status_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
aresponses.add(hostname, '/api', 'get', core_mock)
aresponses.add(hostname, '/apis', 'get', apis_mock)
aresponses.add(hostname, '/apis/g1/g1v1', 'get', status_mock)
Expand All @@ -276,7 +276,7 @@ async def test_unknown_api_statuses_escalate_from_old_apis(

core_mock = resp_mocker(return_value=aiohttp.web.json_response({'versions': ['v1']}))
apis_mock = resp_mocker(return_value=aiohttp.web.json_response({'groups': []}))
status_mock = resp_mocker(return_value=aresponses.Response(status=status))
status_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
aresponses.add(hostname, '/api', 'get', core_mock)
aresponses.add(hostname, '/apis', 'get', apis_mock)
aresponses.add(hostname, '/api/v1', 'get', status_mock)
Expand All @@ -296,7 +296,7 @@ async def test_unknown_api_statuses_escalate_from_new_apis(
apis_mock = resp_mocker(return_value=aiohttp.web.json_response({'groups': [
{'name': 'g1', 'preferredVersion': {'version': ''}, 'versions': [{'version': 'g1v1'}]},
]}))
status_mock = resp_mocker(return_value=aresponses.Response(status=status))
status_mock = resp_mocker(return_value=aresponses.Response(status=status, reason='oops'))
aresponses.add(hostname, '/api', 'get', core_mock)
aresponses.add(hostname, '/apis', 'get', apis_mock)
aresponses.add(hostname, '/apis/g1/g1v1', 'get', status_mock)
Expand Down
2 changes: 1 addition & 1 deletion tests/k8s/test_watching_infinitely.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ async def test_exception_escalates(

async def test_infinite_watch_never_exits_normally(
settings, resource, stream, namespace, aresponses):
error = aresponses.Response(status=555)
error = aresponses.Response(status=555, reason='oops')
stream.feed(
STREAM_WITH_ERROR_410GONE, # watching restarted
STREAM_WITH_UNKNOWN_EVENT, # event ignored
Expand Down
2 changes: 1 addition & 1 deletion tests/observation/test_processing_of_resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def group1_empty_mock(resp_mocker, aresponses, hostname, apis_mock):

@pytest.fixture()
def group1_404mock(resp_mocker, aresponses, hostname, apis_mock):
mock = resp_mocker(return_value=aresponses.Response(status=404))
mock = resp_mocker(return_value=aresponses.Response(status=404, reason='oops'))
aresponses.add(hostname, '/apis/group1/version1', 'get', mock)
return mock

Expand Down
2 changes: 1 addition & 1 deletion tests/peering/test_peer_patching.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ async def test_logs_are_logged_when_absent(
caplog.set_level(0)
settings.peering.stealth = stealth
settings.peering.name = 'name0'
patch_mock = resp_mocker(return_value=aresponses.Response(status=404))
patch_mock = resp_mocker(return_value=aresponses.Response(status=404, reason='oops'))
url = peering_resource.get_url(name='name0', namespace=peering_namespace)
aresponses.add(hostname, url, 'patch', patch_mock)

Expand Down
2 changes: 1 addition & 1 deletion tests/reactor/test_patching_inconsistencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ async def test_patching_with_disappearance(

patch = {'spec': {'x': 'y'}, 'status': {'s': 't'}} # irrelevant
url = resource.get_url(namespace=namespace, name='name1')
patch_mock = resp_mocker(return_value=aresponses.Response(status=404))
patch_mock = resp_mocker(return_value=aresponses.Response(status=404, reason='oops'))
aresponses.add(hostname, url, 'patch', patch_mock)

body = Body({'metadata': {'namespace': namespace, 'name': 'name1'}})
Expand Down

0 comments on commit 0941153

Please sign in to comment.