tests: rremove some flakyness from client tests (#2375)

Update test_send_remote_failover_sync_non_transport_exception_error to
remove some flakyness:
- use a custom transport class instead of mocking it because *sometime*
  the send method won't get mocked :O
- then before asserting the transport state give it a bit more slack
  time to the queue to process and flush the data
- Finally retry the check of the stat for the send without error to
  give the queue enough time to process it

Author: xrmx

tests/client/client_tests.py

@@ -274,33 +274,44 @@ def test_send_remote_failover_sync(should_try, sending_elasticapm_client, caplog
     assert not sending_elasticapm_client._transport.state.did_fail()
 
 
-@mock.patch("elasticapm.transport.http.Transport.send")
-@mock.patch("elasticapm.transport.base.TransportState.should_try")
-def test_send_remote_failover_sync_non_transport_exception_error(should_try, http_send, caplog):
-    should_try.return_value = True
-
+@mock.patch("elasticapm.transport.base.TransportState.should_try", return_value=True)
+def test_send_remote_failover_sync_non_transport_exception_error(should_try, caplog):
     client = Client(
         server_url="http://example.com",
         service_name="app_name",
         secret_token="secret",
-        transport_class="elasticapm.transport.http.Transport",
+        transport_class="tests.fixtures.MockSendHTTPTransport",
         metrics_interval="0ms",
         metrics_sets=[],
     )
+
     # test error
-    http_send.side_effect = ValueError("oopsie")
+    client._transport.send_mock.side_effect = ValueError("oopsie")
     with caplog.at_level("ERROR", "elasticapm.transport"):
         client.capture_message("foo", handled=False)
-    client._transport.flush()
+    try:
+        client._transport.flush()
+    except ValueError:
+        # give flush a bit more room because we may take a bit more than the max timeout to flush
+        client._transport._flushed.wait(timeout=1)
     assert client._transport.state.did_fail()
     assert_any_record_contains(caplog.records, "oopsie", "elasticapm.transport")
 
     # test recovery
-    http_send.side_effect = None
+    client._transport.send_mock.side_effect = None
     client.capture_message("foo", handled=False)
-    client.close()
+    try:
+        client._transport.flush()
+    except ValueError:
+        # give flush a bit more room because we may take a bit more than the max timeout to flush
+        client._transport._flushed.wait(timeout=1)
+    # We have a race here with the queue where we would end up checking for did_fail before the message
+    # is being handled by the queue, so sleep a bit and retry to give it enough time
+    retries = 0
+    while client._transport.state.did_fail() and retries < 3:
+        time.sleep(0.1)
+        retries += 1
     assert not client._transport.state.did_fail()
-    client.close()
 
 
 @pytest.mark.parametrize("validating_httpserver", [{"skip_validate": True}], indirect=True)

tests/fixtures.py

@@ -59,6 +59,7 @@
 from elasticapm.conf.constants import SPAN
 from elasticapm.instrumentation import register
 from elasticapm.traces import execution_context
+from elasticapm.transport.http import Transport
 from elasticapm.transport.http_base import HTTPTransportBase
 from elasticapm.utils.threading import ThreadManager
 
@@ -396,6 +397,18 @@ def get_config(self, current_version=None, keys=None):
         return False, None, 30
 
 
+class MockSendHTTPTransport(Transport):
+    """Mocking the send method of the Transport class sometimes fails silently in client tests.
+    After spending some time trying to understand this with no luck just use this class instead."""
+
+    def __init__(self, url, *args, **kwargs):
+        self.send_mock = mock.Mock()
+        super().__init__(url, *args, **kwargs)
+
+    def send(self, data, forced_flush=False, custom_url=None, custom_headers=None):
+        return self.send_mock(data, forced_flush, custom_url, custom_headers)
+
+
 class TempStoreClient(Client):
     def __init__(self, config=None, **inline) -> None:
         inline.setdefault("transport_class", "tests.fixtures.DummyTransport")