-
Notifications
You must be signed in to change notification settings - Fork 105
fix: strip trailing @suffix for method_name in _get_code_ref #407
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: develop
Are you sure you want to change the base?
Conversation
WalkthroughA helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/test_service.py (1)
31-72: Comprehensive test coverage for the new helper method.The four test cases effectively validate all the key behaviors:
- Regular names pass through unchanged
originalnametakes precedence when present- Trailing
@suffixis correctly stripped@symbols inside parameter brackets are preservedOptional enhancement: Consider adding a test for the combined edge case where both parameter brackets with
@and a trailing suffix are present:def test_get_method_name_params_with_suffix(mocked_item, rp_service): """Test @ in params is preserved while trailing suffix is stripped.""" mocked_item.name = "test_email[user@example.com]@group" mocked_item.originalname = None result = rp_service._get_method_name(mocked_item) expect(result == "test_email[user@example.com]") assert_expectations()This would provide explicit confirmation that the logic handles the most complex real-world scenario from pytest-xdist with loadgroup.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pytest_reportportal/service.py(2 hunks)tests/unit/test_service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_service.py (2)
tests/unit/conftest.py (2)
mocked_item(95-104)rp_service(108-113)pytest_reportportal/service.py (1)
_get_method_name(370-393)
🔇 Additional comments (2)
pytest_reportportal/service.py (2)
370-393: Excellent implementation of the suffix-stripping logic!The method correctly handles all the edge cases described in the PR objectives:
- Prefers
originalnamewhen available (for parameterized tests)- Strips trailing
@suffixadded by pytest-xdist's loadgroup- Preserves
@characters inside parameter brackets (e.g.,test_email[user@example.com])The use of
rfindto locate the last bracket and last@ensures that only trailing suffixes are removed, while@symbols within test parameters remain intact.
612-612: Good refactoring to use the centralized helper.Replacing the inline originalname check with
_get_method_name(item)improves maintainability and ensures consistent name resolution logic.
|
Hello @HardNorth , can you take a look into these changes? Thanks! |
Description
When using
pytest-xdistwith--dist=loadgroup, tests withxdist_groupmarkersget
@group_nameappended to theirnodeid(e.g.,test_foo@my_group).For parameterized tests,
item.originalnameis available and correctly used.For non-parameterized tests,
item.originalnameisNone, so the code fallsback to
item.namewhich includes the@suffix. This produces an invalidcode_reflike
path/file.py:test_foo@group, causing the test to not appear in ReportPortal.Note that appending the
@group_nameto the test name is done on purpose in tests withxdist_group, see for further details in here.Comparison with pytest-ibutsu
The pytest-ibutsu plugin handles this
correctly by using
item.location[2]which returns the original function namewithout any xdist suffixes:
pytest-ibutsu/src/pytest_ibutsu/modeling.py:
This PR aligns pytest-reportportal behavior with pytest-ibutsu.
Solution
Add
_get_method_name()method that:item.originalnameif available@suffixfromitem.name@inside parameter brackets (e.g.,test_email[user@example.com])Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.