feat(query): add --test-target filter for test items by parent target#1
feat(query): add --test-target filter for test items by parent target#1
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded an optional CLI flag Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
actor User
end
rect rgba(200,255,200,0.5)
participant CLI
participant QueryHandler
participant APIClient
end
User->>CLI: Run `query` with --test-target
CLI->>QueryHandler: Parse options (test_target)
alt test_target provided
QueryHandler->>APIClient: get_test_items(type='TEST', name=test_target)
APIClient-->>QueryHandler: matching test items (or empty)
alt match found
QueryHandler->>QueryHandler: extract parent_id
QueryHandler->>APIClient: get_test_items(for launch, other filters)
APIClient-->>QueryHandler: returned items
QueryHandler->>QueryHandler: filter items where item.parent == parent_id
QueryHandler-->>CLI: return filtered items
else no match
QueryHandler-->>CLI: log error, exit code 1
end
else no test_target
QueryHandler->>APIClient: get_test_items(for launch, other filters)
APIClient-->>QueryHandler: returned items
QueryHandler-->>CLI: return items
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
src/reportportal/rp_query.py (1)
505-517: Consider warning when--test-targetis provided without a launch context.If the user specifies
--test-targetwithout--launch-idor--launch-name, the filter is silently ignored since the code proceeds to list launches instead. This could be confusing for users who expect filtering to apply.💡 Suggested improvement to warn users
# Resolve test target to parent ID if specified test_target = getattr(options, 'test_target', None) + if test_target and not options.launch_id: + logger.warning("--test-target is only applicable when querying test items (requires --launch-id or --launch-name)") if test_target and options.launch_id: # Find the TEST item matching the target name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reportportal/rp_query.py` around lines 505 - 517, If a --test-target is provided but no launch context is given, add a clear warning so the filter won't be silently ignored: when reading test_target from options (variable test_target) check if not (options.launch_id or options.launch_name) and emit logger.warning stating that test_target filtering will be ignored unless a launch (launch_id or launch_name) is provided; keep the existing branch that resolves parent_id only when options.launch_id is present (the block using client.get_test_items and parent_items) but ensure the warning is logged before the code proceeds to list launches or otherwise continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reportportal/rp_query.py`:
- Around line 532-535: The code filters 'items' when 'test_target' and
'parent_id' without first ensuring 'items' is not None, so move or add a guard
to check items is not None (the result of fetch_test_items()) before any
iteration; specifically, in the function handling fetch_test_items (where the
variables items, test_target, parent_id are used) ensure you test "if items is
None" and handle/return appropriately before performing the list comprehension
filtering (items = [item for item in items if item.get('parent') == parent_id])
so you never attempt to iterate over None.
---
Nitpick comments:
In `@src/reportportal/rp_query.py`:
- Around line 505-517: If a --test-target is provided but no launch context is
given, add a clear warning so the filter won't be silently ignored: when reading
test_target from options (variable test_target) check if not (options.launch_id
or options.launch_name) and emit logger.warning stating that test_target
filtering will be ignored unless a launch (launch_id or launch_name) is
provided; keep the existing branch that resolves parent_id only when
options.launch_id is present (the block using client.get_test_items and
parent_items) but ensure the warning is logged before the code proceeds to list
launches or otherwise continue.
🪄 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: 48b270e1-7f9f-4732-b67b-373777836888
📒 Files selected for processing (2)
src/reportportal/ap.pysrc/reportportal/rp_query.py
| filters_desc = _build_filter_description(options) | ||
| filter_msg = f" with filters: {', '.join(filters_desc)}" if filters_desc else "" | ||
|
|
||
| # Resolve test target to parent ID if specified |
There was a problem hiding this comment.
Could you please refactor this part as its own function, and do just a function call here. Or maybe, move the call inside the if options.launch_id block line 519.
First I know this is originally my code, and I honestly have to say, it is a spaghetti code already. And structural refactoring is on the list. This would be a perfect beginning, of making the code more structured.
| test_target = getattr(options, 'test_target', None) | ||
| if test_target and options.launch_id: | ||
| # Find the TEST item matching the target name | ||
| parent_items = client.get_test_items( |
There was a problem hiding this comment.
Please add TODO comment here about rewriting it to fetch_test_item, once it supports additional filters.
you may link issue #2
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/reportportal/rp_query.py`:
- Around line 466-487: The return type annotation on _resolve_test_target is
incorrect: change its signature to return Optional[str] (not Optional[int]) and
ensure the variable parent_id and returned value remain strings as produced by
client.get_test_items (refer to _resolve_test_target, client.get_test_items and
parent_id); update the docstring return type description if present to reflect a
string parent item ID.
🪄 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: 0712fdb0-5e75-45af-bd09-1288d2d1e3ca
📒 Files selected for processing (2)
src/reportportal/ap.pysrc/reportportal/rp_query.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/reportportal/ap.py
… name Signed-off-by: silvi-t <silvinka.tarabova@gmail.com>
zkraus
left a comment
zkraus
left a comment
There was a problem hiding this comment.
All points addressed, LGTM.
2 participants
Summary
--test-targetCLI option to thequerysubcommand for filtering test items by parent test target name--test-target kuadrant)Summary by CodeRabbit