From 70caea57e88d15f8866fa4733d6d68e285d54562 Mon Sep 17 00:00:00 2001 From: Daniel McCoy Stephenson Date: Fri, 6 Nov 2026 21:19:31 -0700 Subject: [PATCH] Name the argument when the constructor is given a non-string A truthy non-string base_url, application, version, key or install_id raised AttributeError from .strip() with no hint of which argument was wrong. They now raise a ValueError naming the argument and the type it got, never the value, so a rejected key does not reach a log. Falsy values keep their meaning: they never reached .strip(), so a program passing one already ran. Also wait for the sender to take the first report in test_a_dropped_report_is_logged_at_debug_with_its_name. If it woke after the last filler was dropped, the overflow event found a free slot and was queued, failing the test under load. Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +++++-- tests/test_trace_client.py | 45 +++++++++++++++++++++++++++++++++--- trace_client/trace_client.py | 11 ++++++++- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index bba38b9..6956fcc 100644 --- a/README.md +++ b/README.md @@ -25,8 +25,12 @@ trace.close() ## Every event carries the program's version The third argument to `TraceClient` is the program's own version, and it is -required: a missing or blank one, or one over 255 characters after trimming, -raises `ValueError`. Every event the client sends — `startup`, `command`, +required: a missing or blank one, one over 255 characters after trimming, or +one that is not a string, raises `ValueError`. So does a value that is not a +string for `base_url`, `application`, `key` or `install_id=`, naming the +argument, whether or not the client ends up enabled, so a settings file that +turns `version: 1.4` into a number is caught at startup. Every event the +client sends — `startup`, `command`, anything else — carries it as the tag `version`, so every event can be tied to a release, not just `startup`. An event that passes its own `version` tag keeps it, and the dict passed as `tags` is never modified. There is no need diff --git a/tests/test_trace_client.py b/tests/test_trace_client.py index 50afd95..b9aa1ed 100644 --- a/tests/test_trace_client.py +++ b/tests/test_trace_client.py @@ -468,6 +468,40 @@ def test_constructor_errors_name_the_argument_they_reject(self): TraceClient(self.base_url, "MyGame", overlong) self.assertIn(str(MAX_TAG_LENGTH), str(raised.exception), "an overlong value names the limit") + def test_a_value_that_is_not_a_string_is_rejected_by_the_argument_it_was_given_for(self): + cases = ( + ("base_url", "int", lambda **kw: TraceClient(5, "MyGame", "1.2.3", **kw)), + ("application", "int", lambda **kw: TraceClient(self.base_url, 5, "1.2.3", **kw)), + ("application", "bytes", lambda **kw: TraceClient(self.base_url, b"MyGame", "1.2.3", **kw)), + ("version", "float", lambda **kw: TraceClient(self.base_url, "MyGame", 1.4, **kw)), + ("key", "int", lambda **kw: TraceClient(self.base_url, "MyGame", "1.2.3", key=123, **kw)), + ("install_id", "int", lambda **kw: TraceClient(self.base_url, "MyGame", "1.2.3", install_id=7, **kw)), + ) + for argument, type_name, construct in cases: + for options in ({}, {"enabled": False}): + with self.assertRaises(ValueError, msg=argument) as raised: + construct(**options) + self.assertEqual("%s must be a string, not %s" % (argument, type_name), str(raised.exception), + "the only raising path should say which argument was wrong") + + def test_a_rejected_key_is_not_repeated_in_the_error(self): + with self.assertRaises(ValueError) as raised: + TraceClient(self.base_url, "MyGame", "1.2.3", key=123456789) + self.assertNotIn("123456789", str(raised.exception), "an error can end up in a log; a key should not") + + def test_an_empty_value_of_another_type_keeps_its_old_meaning(self): + # These never reached .strip(), so a program passing one already ran. + for empty in (0, False, []): + client = TraceClient(self.base_url, "MyGame", "1.2.3", key=empty, enabled=False) + self.assertEqual("config", client.disabled_reason, repr(empty)) + self.assertEqual("no key", TraceClient(self.base_url, "MyGame", "1.2.3", key=empty).disabled_reason) + client = TraceClient(self.base_url, "MyGame", "1.2.3", key="k", install_id=empty) + self.assertIsNone(client.install_id, repr(empty)) + client.close() + with self.assertRaises(ValueError, msg=repr(empty)) as raised: + TraceClient(self.base_url, "MyGame", empty) + self.assertEqual("version is required", str(raised.exception)) + def test_the_sender_is_one_daemon_thread_named_after_the_application(self): before = set(threading.enumerate()) client = TraceClient(self.base_url, " MyGame ", "1.2.3", key="k") @@ -569,9 +603,14 @@ def test_queue_is_bounded_and_drops_rather_than_grows(self): def test_a_dropped_report_is_logged_at_debug_with_its_name(self): self.capture.release.clear() # hold the sender on the first report client = TraceClient(self.base_url, "MyGame", "1.2.3", key="k") - # The queue holds QUEUE_CAPACITY and the sender at most one more, so - # the next report has nowhere to go whatever the sender has picked up. - for _ in range(TraceClient.QUEUE_CAPACITY + 1): + client.report("filler") + # Wait for the sender to take it: if it woke up between the last filler + # and the overflow, the overflow would find a free slot and not be dropped. + deadline = time.monotonic() + 5 + while time.monotonic() < deadline and not client._queue.empty(): + time.sleep(0.01) + self.assertTrue(client._queue.empty(), "the sender should have picked up the first report") + for _ in range(TraceClient.QUEUE_CAPACITY): client.report("filler") client.report("overflow-event") dropped = [r for r in self.log if r.getMessage() == "[trace] queue full, dropped overflow-event"] diff --git a/trace_client/trace_client.py b/trace_client/trace_client.py index 9484b92..ba13e8f 100644 --- a/trace_client/trace_client.py +++ b/trace_client/trace_client.py @@ -139,7 +139,16 @@ def __init__(self, base_url: str, application: str, version: str, *, key: Option longer than :data:`MAX_TAG_LENGTH` characters is a :class:`ValueError`. ``install_id_file`` is a path to load it from (or create it in) with :meth:`install_id_from_file`, only when the - client is enabled; an explicit ``install_id`` wins over it.""" + client is enabled; an explicit ``install_id`` wins over it. + + A value that is not a string for ``base_url``, ``application``, + ``version``, ``key`` or ``install_id`` is a :class:`ValueError` naming + the argument, even on a client that would be disabled: a settings + file that reads ``version: 1.4`` as a number should say so.""" + for argument, given in (("base_url", base_url), ("application", application), ("version", version), + ("key", key), ("install_id", install_id)): + if given and not isinstance(given, str): + raise ValueError("%s must be a string, not %s" % (argument, type(given).__name__)) if not base_url or not base_url.strip(): raise ValueError("base_url is required") if not application or not application.strip():