drop unit tests in favor of functional tests

botocore/handlers.py

@@ -153,7 +153,9 @@ def check_for_200_error(response, **kwargs):
         # trying to retrieve the response.  See Endpoint._get_response().
         return
     http_response, parsed = response
-    if _looks_like_special_case_error(http_response):
+    if _looks_like_special_case_error(
+        http_response.status_code, http_response.content
+    ):
         logger.debug(
             "Error found for response with 200 status code, "
             "errors: %s, changing status code to "
@@ -1343,7 +1345,7 @@ def _update_status_code(response, **kwargs):
     ('before-call.ec2.CopySnapshot', inject_presigned_url_ec2),
     ('request-created', add_retry_headers),
     ('request-created.machinelearning.Predict', switch_host_machinelearning),
-    ('needs-retry.s3.*', _retry_200_error, REGISTER_FIRST),
+    ('needs-retry.s3.*', _update_status_code, REGISTER_FIRST),
     ('choose-signer.cognito-identity.GetId', disable_signing),
     ('choose-signer.cognito-identity.GetOpenIdToken', disable_signing),
     ('choose-signer.cognito-identity.UnlinkIdentity', disable_signing),

tests/functional/test_s3.py

@@ -460,7 +460,20 @@ def test_s3_copy_object_with_incomplete_response(self):
         self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
         self.assertTrue("CopyObjectResult" in response)
 
-    def test_s3_copy_object_with_200_error_response(self):
+
+class TestS3200ErrorResponse(BaseS3OperationTest):
+    def create_s3_client(self, **kwargs):
+        client_kwargs = {"region_name": self.region}
+        client_kwargs.update(kwargs)
+        return self.session.create_client("s3", **client_kwargs)
+
+    def create_stubbed_s3_client(self, **kwargs):
+        client = self.create_s3_client(**kwargs)
+        http_stubber = ClientHTTPStubber(client)
+        http_stubber.start()
+        return client, http_stubber
+
+    def test_s3_200_with_error_response(self):
         self.client, self.http_stubber = self.create_stubbed_s3_client(
             region_name="us-east-1"
         )
@@ -470,7 +483,8 @@ def test_s3_copy_object_with_200_error_response(self):
             b"<Message>Please reduce your request rate.</Message>"
             b"</Error>"
         )
-        # Populate 5 retries for SlowDown
+        # Populate 5 attempts for SlowDown to validate
+        # we reached four max retries and raised an exception.
         for i in range(5):
             self.http_stubber.add_response(status=200, body=error_body)
         with self.assertRaises(botocore.exceptions.ClientError) as context:
@@ -488,6 +502,45 @@ def test_s3_copy_object_with_200_error_response(self):
             context.exception.response["Error"]["Code"], "SlowDown"
         )
 
+    def test_s3_200_with_no_error_response(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200, body=b"<NotAnError/>")
+
+        response = self.client.copy_object(
+            Bucket="bucket",
+            CopySource="other-bucket/test.txt",
+            Key="test.txt",
+        )
+
+        # Validate that the status code remains 200.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
+
+    def test_s3_200_with_error_response_on_streaming_operation(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200, body=b"<Error/>")
+        response = self.client.get_object(Bucket="bucket", Key="test.txt")
+
+        # Validate that the status code remains 200 because we don't
+        # process 200-with-error responses on streaming operations.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
+
+    def test_s3_200_response_with_no_body(self):
+        self.client, self.http_stubber = self.create_stubbed_s3_client(
+            region_name="us-east-1"
+        )
+        self.http_stubber.add_response(status=200)
+        response = self.client.head_object(Bucket="bucket", Key="test.txt")
+
+        # Validate that the status code remains 200 on operations without a body.
+        self.assertEqual(len(self.http_stubber.requests), 1)
+        self.assertEqual(response["ResponseMetadata"]["HTTPStatusCode"], 200)
+
 
 class TestAccesspointArn(BaseS3ClientConfigurationTest):
     def setUp(self):

tests/unit/test_handlers.py

@@ -501,6 +501,33 @@ def test_presigned_url_casing_changed_for_rds(self):
         )
         self.assertIn('X-Amz-Signature', params['PreSignedUrl'])
 
+    def test_500_status_code_set_for_200_response(self):
+        http_response = mock.Mock()
+        http_response.status_code = 200
+        http_response.content = """
+             <Error>
+               <Code>AccessDenied</Code>
+               <Message>Access Denied</Message>
+               <RequestId>id</RequestId>
+               <HostId>hostid</HostId>
+             </Error>
+         """
+        handlers.check_for_200_error((http_response, {}))
+        self.assertEqual(http_response.status_code, 500)
+
+    def test_200_response_with_no_error_left_untouched(self):
+        http_response = mock.Mock()
+        http_response.status_code = 200
+        http_response.content = "<NotAnError></NotAnError>"
+        handlers.check_for_200_error((http_response, {}))
+        # We don't touch the status code since there are no errors present.
+        self.assertEqual(http_response.status_code, 200)
+
+    def test_500_response_can_be_none(self):
+        # A 500 response can raise an exception, which means the response
+        # object is None.  We need to handle this case.
+        handlers.check_for_200_error(None)
+
     def test_route53_resource_id(self):
         event = 'before-parameter-build.route53.GetHostedZone'
         params = {
@@ -1175,14 +1202,14 @@ def test_s3_special_case_is_before_other_retry(self):
             caught_exception=None,
         )
         # This is implementation specific, but we're trying to verify that
-        # the _retry_200_error is before any of the retry logic in
+        # the _update_status_code is before any of the retry logic in
         # botocore.retryhandlers.
         # Technically, as long as the relative order is preserved, we don't
         # care about the absolute order.
         names = self.get_handler_names(responses)
-        self.assertIn('_retry_200_error', names)
+        self.assertIn('_update_status_code', names)
         self.assertIn('RetryHandler', names)
-        s3_200_handler = names.index('_retry_200_error')
+        s3_200_handler = names.index('_update_status_code')
         general_retry_handler = names.index('RetryHandler')
         self.assertTrue(
             s3_200_handler < general_retry_handler,
@@ -1917,80 +1944,3 @@ def test_document_response_params_without_expires(document_expires_mocks):
     mocks['section'].get_section.assert_not_called()
     mocks['param_section'].add_new_section.assert_not_called()
     mocks['doc_section'].write.assert_not_called()
-
-
-@pytest.fixture()
-def operation_model_for_200_error():
-    operation_model = mock.Mock()
-    operation_model.has_streaming_output = False
-    return operation_model
-
-
-@pytest.fixture()
-def response_dict_for_200_error():
-    return {
-        'status_code': 200,
-        'body': (
-            b"<Error>"
-            b"<Code>SlowDown</Code>"
-            b"<Message>Please reduce your request rate.</Message>"
-            b"</Error>"
-        ),
-    }
-
-
-def test_500_status_code_set_for_200_response(
-    operation_model_for_200_error, response_dict_for_200_error
-):
-    handlers._handle_200_error(
-        operation_model_for_200_error, response_dict_for_200_error
-    )
-    assert response_dict_for_200_error['status_code'] == 500
-
-
-def test_200_response_with_no_error_left_untouched(
-    operation_model_for_200_error, response_dict_for_200_error
-):
-    response_dict_for_200_error['body'] = b"<NotAnError/>"
-    handlers._handle_200_error(
-        operation_model_for_200_error, response_dict_for_200_error
-    )
-    # We don't touch the status code since there are no errors present.
-    assert response_dict_for_200_error['status_code'] == 200
-
-
-def test_200_response_with_streaming_output_left_untouched(
-    operation_model_for_200_error,
-    response_dict_for_200_error,
-):
-    operation_model_for_200_error.has_streaming_output = True
-    handlers._handle_200_error(
-        operation_model_for_200_error, response_dict_for_200_error
-    )
-    # We don't touch the status code on streaming operations.
-    assert response_dict_for_200_error['status_code'] == 200
-
-
-def test_200_response_with_no_body_left_untouched(
-    operation_model_for_200_error, response_dict_for_200_error
-):
-    response_dict_for_200_error['body'] = b""
-    handlers._handle_200_error(
-        operation_model_for_200_error, response_dict_for_200_error
-    )
-    assert response_dict_for_200_error['status_code'] == 200
-
-
-def test_http_status_code_updated_to_retry_200_response():
-    http_response = mock.Mock()
-    http_response.status_code = 200
-    parsed = {}
-    parsed.setdefault('ResponseMetadata', {})['HTTPStatusCode'] = 500
-    handlers._retry_200_error((http_response, parsed))
-    assert http_response.status_code == 500
-
-
-def test_500_response_can_be_none():
-    # A 500 response can raise an exception, which means the response
-    # object is None.  We need to handle this case.
-    handlers._retry_200_error(None)