-
Notifications
You must be signed in to change notification settings - Fork 20
bugsnag: strip JSON objects from grouping strings, normalize LauncherError #261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,6 +184,52 @@ def test_strips_object_repr(self): | |
| ) | ||
|
|
||
|
|
||
| class TestNormalizeLauncherError: | ||
| def _make_launcher_error( | ||
| self, message: str, cause: BaseException | None = None | ||
| ) -> Exception: | ||
| try: | ||
| from cloud_pipelines_backend.launchers.interfaces import LauncherError | ||
| except ImportError: | ||
| pytest.skip("LauncherError not importable") | ||
| if cause: | ||
| try: | ||
| raise LauncherError(message) from cause | ||
| except LauncherError as exc: | ||
| return exc | ||
| return LauncherError(message) | ||
|
|
||
| def test_strips_pod_spec_json(self): | ||
| pod_spec = ( | ||
| "{'apiVersion': 'v1', 'kind': 'Pod', 'metadata': {'name': 'task-abc-xyz'}}" | ||
| ) | ||
| exc = self._make_launcher_error(f"Failed to create pod: {pod_spec}") | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "LauncherError: Failed to create pod: {...}" | ||
|
|
||
| def test_with_timeout_cause(self): | ||
| cause = TimeoutError("The read operation timed out") | ||
| exc = self._make_launcher_error( | ||
| "Failed to create pod: {'apiVersion': 'v1'}", cause=cause | ||
| ) | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "LauncherError: Failed to create pod: {...}" | ||
|
|
||
| def test_no_colon_in_message(self): | ||
| exc = self._make_launcher_error("launch failed") | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "LauncherError: launch failed" | ||
|
|
||
| def test_multi_colon_diagnostic_preserved(self): | ||
| exc = self._make_launcher_error( | ||
| "creating pod: spec invalid: missing field 'name'" | ||
| ) | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert ( | ||
| result == "LauncherError: creating pod: spec invalid: missing field 'name'" | ||
| ) | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider adding a test for multi-colon The three new tests cover JSON suffix, cause chain, and no-colon. None covers the case where there is real diagnostic info after the first colon — exactly the scenario where the verb-phrase truncation silently degrades grouping quality (see the other comment on Adding: def test_multiple_colons_in_message(self):
exc = self._make_launcher_error("creating pod: spec invalid: missing field 'name'")
result = error_normalization.normalize_error_message(exception=exc)
assert result == "LauncherError: creating pod" # or whatever the desired behavior is…either pins the truncation as intentional or surfaces the regression so the policy gets revisited.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Added |
||
| class TestFallback: | ||
| def test_strips_hex_address(self): | ||
| exc = ValueError("object at 0xdeadbeef failed") | ||
|
|
@@ -204,3 +250,13 @@ def test_stable_message_unchanged(self): | |
| exc = AttributeError("'NoneType' object has no attribute 'encode'") | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "AttributeError: 'NoneType' object has no attribute 'encode'" | ||
|
|
||
| def test_strips_json_object(self): | ||
| exc = RuntimeError("operation failed: {'key': 'value', 'nested': {'a': 1}}") | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "RuntimeError: operation failed: {...}" | ||
|
|
||
| def test_strips_json_object_double_quotes(self): | ||
| exc = RuntimeError('operation failed: {"key": "value"}') | ||
| result = error_normalization.normalize_error_message(exception=exc) | ||
| assert result == "RuntimeError: operation failed: {...}" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should literally replace the
_JSON_OBJECT_PATTERNwith{...}and not just...correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Yes — the pattern replaces the matched span with
{...}, which includes the opening{. So the result is{...}not....Also improved the pattern to handle optional whitespace after
{(e.g.{ "key": ...}) and updated the comment to document that the greedy match intentionally strips everything from the first dict/JSON literal to end-of-string.