tests: add unit tests for app/utils/tool_trace.py and app/utils/traci…#2630
tests: add unit tests for app/utils/tool_trace.py and app/utils/traci…#2630HaMZAAsif043 wants to merge 5 commits into
Conversation
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds unit tests to validate no-op tracing behavior and tool-trace formatting/redaction utilities.
Changes:
- Introduces tests for
traceabledecorator semantics and metadata preservation. - Adds redaction recursion/precedence tests for sensitive fields and runtime objects.
- Adds formatting/truncation tests for JSON previews and tool trace entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/utils/test_tracing.py | Verifies traceable behaves as a no-op decorator and preserves function metadata. |
| tests/utils/test_tool_trace.py | Validates redaction, JSON preview formatting, and tool trace entry rendering/truncation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_traceable_returns_identity_decorator() -> None: | ||
| def traced_function() -> str: | ||
| return "ok" | ||
|
|
||
| assert traceable()(traced_function) is traced_function | ||
| assert traceable("investigation-step", extra="x")(traced_function) is traced_function | ||
|
|
||
|
|
| for scalar in (123, "plain", None, True): | ||
| assert redact_sensitive(scalar) is scalar |
| assert redacted["items"] == [ | ||
| {"password": "[redacted]"}, | ||
| ["public", {"_client": "[runtime object]"}], | ||
| ] |
|
|
||
| def test_format_tool_trace_entry_handles_empty_trace_record_and_output_limit() -> None: | ||
| formatted = format_tool_trace_entry({}) | ||
| assert formatted == "- `tool` (iteration None)\n input: `{}`\n output: `null`" |
Greptile SummaryThis PR adds unit tests for
Confidence Score: 5/5Test-only addition with no changes to production code; safe to merge. The change is purely additive tests. All assertions are correct and will pass against the current implementation. The two looseness findings are minor precision nits that do not affect correctness or coverage of the intended behaviour. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[test_redact_sensitive_recurses_into_nested_collections] --> R[redact_sensitive]
B[test_redact_sensitive_handles_scalars_and_regex_precedence] --> R
C[test_format_json_preview_redacts_truncates_and_stringifies] --> FJP[format_json_preview]
FJP --> R
D[test_format_tool_trace_entry_populates_fields_and_collapses_previews] --> FTTE[format_tool_trace_entry]
E[test_format_tool_trace_entry_handles_empty_trace_record_and_output_limit] --> FTTE
FTTE --> FJP
F[test_traceable_returns_identity_decorator] --> T[traceable]
G[test_traceable_preserves_args_kwargs_return_value_and_metadata] --> T
Reviews (6): Last reviewed commit: "style: ruff format test_tool_trace.py" | Re-trigger Greptile |
|
Could you please take a look at the Greptile and Copilot comments? |
yes on it |
|
@greptile review |
|
@greptile review |
|
|
|
@greptile review |
1 similar comment
|
@greptile review |
|
Thanks for fixing most of the comments I still think the "is traced_function" checks are a bit too strict. A small "functools.wraps" no-op wrapper could still be valid but would fail these tests. Could you either remove the "is" checks or add a short note saying object identity is intentionally part of the contract? |
|
Added a comment clarifying that object identity is intentional here |
… here raceable is a no-op and should return the original function, not a wrapper.
|
@greptile review |
tests: add unit tests for app/utils/tool_trace.py and app/utils/tracing.py
Fixes #2594
Describe the changes you have made in this PR -
Added comprehensive unit tests for
app/utils/tool_trace.pyandapp/utils/tracing.pyinsidetests/utils/. The tests verify:traceabledecorator acts as a no-op identity decorator preserving metadata.Demo/Screenshot for feature changes and bug fixes -
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.