feat(query): add --show-logs flag to display error logs for failed tests - #17
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesError Log Display Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reportportal/rp_query.py (1)
651-662:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same local filter set to failure-log output.
Line 660 currently calls
_output_failed_logs(items, client)with the unfiltered list, so--show-logscan print failures that were filtered out from the displayed results (--name-regex,--attribute*). It also conflicts with the--names-onlycontract.Suggested patch
- if filter_opts['show_logs']: - _output_failed_logs(items, client) + if filter_opts['show_logs'] and not filter_opts['names_only']: + filtered_for_logs = apply_all_filters( + items, + name_regex=filter_opts['name_regex'], + attribute_filters=filter_opts['attribute_filters'], + attribute_regex_filters=filter_opts['attribute_regex_filters'], + ) + if filtered_for_logs is None: + logger.error("Failed to apply filters for log output") + return 1 + _output_failed_logs(filtered_for_logs, client)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reportportal/rp_query.py` around lines 651 - 662, list_test_items is being called but its filtered result isn't used, so _output_failed_logs(items, client) still receives the unfiltered list; change the call to capture the filtered set (e.g. filtered_items = list_test_items(items, name_regex=..., attribute_filters=..., attribute_regex_filters=..., show_attributes=..., names_only=...)) and then call _output_failed_logs(filtered_items, client) so the same local filters (including names_only) are applied to the failure-log output; update any subsequent logic that assumed the original items to use filtered_items instead.
🧹 Nitpick comments (1)
tests/unit/test_rp_query_logs.py (1)
40-234: ⚡ Quick winAdd regression coverage for
show_logsintegration with local filters andnames_only.This suite validates helper behaviour well, but it doesn’t cover the
run_querypath whereshow_logscombines with local filters (name_regex/attribute*) ornames_only. A focused integration-style unit test there would lock the intended CLI behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_rp_query_logs.py` around lines 40 - 234, Add an integration-style unit test that exercises run_query with show_logs enabled and local filters to ensure show_logs respects name_regex/attribute filters and the names_only flag: call run_query (or the CLI entry that invokes it) with mock_client, show_logs=True, a name_regex and an attribute filter (e.g., attributeX=value) and verify mock_client.get_test_item_by_id/get_logs are only invoked for matching items and output contains only those tests; also add a separate case setting names_only=True and assert output prints only test names (no log bodies). Use the existing test fixtures/mocks and reference the run_query function, the show_logs parameter, name_regex, names_only, and the client methods get_test_item_by_id/get_logs to locate where to add the new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reportportal/rp_query.py`:
- Around line 498-501: The helper _truncate_message currently calls len(message)
and slices it directly, which will raise if message is None or not a string;
update _truncate_message (and the similar call around line 516) to first
normalise the input by turning falsy/None into an empty string and otherwise
casting to str (e.g., msg = '' if message is None else str(message)), then
perform the length check and slicing on that normalized msg so truncation never
raises on non-string inputs.
- Line 576: The print call uses an unnecessary f-string prefix: replace the
print(f" (no error logs found)") invocation with a normal string literal
(print(" (no error logs found)")) so the f prefix is removed; update the print
statement where print(f" (no error logs found)") appears in rp_query.py.
---
Outside diff comments:
In `@src/reportportal/rp_query.py`:
- Around line 651-662: list_test_items is being called but its filtered result
isn't used, so _output_failed_logs(items, client) still receives the unfiltered
list; change the call to capture the filtered set (e.g. filtered_items =
list_test_items(items, name_regex=..., attribute_filters=...,
attribute_regex_filters=..., show_attributes=..., names_only=...)) and then call
_output_failed_logs(filtered_items, client) so the same local filters (including
names_only) are applied to the failure-log output; update any subsequent logic
that assumed the original items to use filtered_items instead.
---
Nitpick comments:
In `@tests/unit/test_rp_query_logs.py`:
- Around line 40-234: Add an integration-style unit test that exercises
run_query with show_logs enabled and local filters to ensure show_logs respects
name_regex/attribute filters and the names_only flag: call run_query (or the CLI
entry that invokes it) with mock_client, show_logs=True, a name_regex and an
attribute filter (e.g., attributeX=value) and verify
mock_client.get_test_item_by_id/get_logs are only invoked for matching items and
output contains only those tests; also add a separate case setting
names_only=True and assert output prints only test names (no log bodies). Use
the existing test fixtures/mocks and reference the run_query function, the
show_logs parameter, name_regex, names_only, and the client methods
get_test_item_by_id/get_logs to locate where to add the new tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b97da88a-9528-4054-bbb2-77f7fa40b475
📒 Files selected for processing (4)
src/reportportal/ap.pysrc/reportportal/rp_api_client.pysrc/reportportal/rp_query.pytests/unit/test_rp_query_logs.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_rp_query_logs.py (1)
138-235: ⚡ Quick winAdd a retry-path test for failed attempts with empty log payloads.
There’s good coverage for retry status rendering, but not for the empty-logs case in failed attempts. A focused test here will prevent silent blank output regressions in the retry branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_rp_query_logs.py` around lines 138 - 235, Add a unit test in tests/unit/test_rp_query_logs.py that exercises the retry branch when some attempts return empty log payloads: create an item with retries via mock_client.get_test_item_by_id (first returning {'id': 'item1','retries':[{'id':'r1'},{'id':'r2'}], then each retry detail), stub mock_client.get_logs to return [] for one or more retry attempts and a non-empty final attempt (e.g. [] , [], [{'message':'final error','level':'ERROR'}]), call _output_failed_logs(items, mock_client), and assert the output still shows the attempts count and labels (e.g. 'Attempt 1', 'Attempt 2', 'Attempt 3') and that the final error message is present while empty attempts do not produce blank/unlabeled output; ensure it follows the patterns used in existing tests (test_retries_shown_as_attempts, mock_client.get_test_item_by_id, mock_client.get_logs).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/reportportal/rp_query.py`:
- Around line 569-572: The retry-path currently leaves a blank block when a
failed attempt has no ERROR logs; update the branch that calls
_fetch_error_logs(attempt.get('id'), client) inside the if status ==
utils.STATUS_FAILED check so that after fetching logs you call _print_logs(logs,
indent=" ") if logs is non-empty, otherwise print an explicit placeholder
message (e.g. " No ERROR logs found for this attempt") using the same indent
so the retry display mirrors the non-retry behavior; ensure this change
references the existing functions _fetch_error_logs and _print_logs and the
attempt/status variables.
---
Nitpick comments:
In `@tests/unit/test_rp_query_logs.py`:
- Around line 138-235: Add a unit test in tests/unit/test_rp_query_logs.py that
exercises the retry branch when some attempts return empty log payloads: create
an item with retries via mock_client.get_test_item_by_id (first returning {'id':
'item1','retries':[{'id':'r1'},{'id':'r2'}], then each retry detail), stub
mock_client.get_logs to return [] for one or more retry attempts and a non-empty
final attempt (e.g. [] , [], [{'message':'final error','level':'ERROR'}]), call
_output_failed_logs(items, mock_client), and assert the output still shows the
attempts count and labels (e.g. 'Attempt 1', 'Attempt 2', 'Attempt 3') and that
the final error message is present while empty attempts do not produce
blank/unlabeled output; ensure it follows the patterns used in existing tests
(test_retries_shown_as_attempts, mock_client.get_test_item_by_id,
mock_client.get_logs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c68efc8e-a626-47a5-a9d3-b6ca44a38684
📒 Files selected for processing (4)
src/reportportal/ap.pysrc/reportportal/rp_api_client.pysrc/reportportal/rp_query.pytests/unit/test_rp_query_logs.py
✅ Files skipped from review due to trivial changes (1)
- src/reportportal/ap.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/reportportal/rp_api_client.py
Signed-off-by: Silvia Tarabova <starabov@redhat.com>
2 participants
Summary
--show-logsflag torptool querythat fetches and displays ERROR-level logs for failed STEP itemsTest plan
rptool query --rp-project nightly-testsuite --launch-name "nightly-all #633" --show-logs— verify error logs appear for failed tests--name <test>to confirm logs display for a single filtered testpytest tests/unit/test_rp_query_logs.py(13 tests)Summary by CodeRabbit
New Features
Tests