refactor(tests): auto-convert array-api-strict modules#5528
Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
📝 WalkthroughWalkthroughThis PR refactors the array_api_strict testing module to centralize object conversion logic. Instead of individual ChangesCentral conversion framework
Descriptor module simplification and centralization
Utility modules simplification
Fitting module simplification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/array_api_strict/common.py (1)
160-167: 💤 Low valueMinor: double conversion when
keep_converting=True.When
convertedis anArrayAPIList, callingconverted.append(converted_item)invokes the overriddenappendthat callsconvert_array_api_strict_valueagain on an already-converted item. The conversion is idempotent so this is correct, but wasteful.♻️ Optional: bypass the override for initial population
def _try_convert_list(value: list[Any], *, keep_converting: bool = False) -> list[Any]: converted = ArrayAPIList() if keep_converting else [] changed = keep_converting for item in value: converted_item = convert_array_api_strict_value(item) - converted.append(converted_item) + list.append(converted, converted_item) changed = changed or converted_item is not item return converted if changed else value🤖 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 `@source/tests/array_api_strict/common.py` around lines 160 - 167, The loop in _try_convert_list uses converted.append on an ArrayAPIList when keep_converting=True, triggering ArrayAPIList.append which re-calls convert_array_api_strict_value and causes a redundant conversion; fix by collecting converted items into a temporary plain list (e.g., temp = []) inside the loop (use temp.append(converted_item) and update changed there), and after the loop, if keep_converting is True build the ArrayAPIList once from that temp (converted = ArrayAPIList(temp)) so the overridden append is not invoked for each item; leave the return logic (return converted if changed else value) unchanged.
🤖 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.
Nitpick comments:
In `@source/tests/array_api_strict/common.py`:
- Around line 160-167: The loop in _try_convert_list uses converted.append on an
ArrayAPIList when keep_converting=True, triggering ArrayAPIList.append which
re-calls convert_array_api_strict_value and causes a redundant conversion; fix
by collecting converted items into a temporary plain list (e.g., temp = [])
inside the loop (use temp.append(converted_item) and update changed there), and
after the loop, if keep_converting is True build the ArrayAPIList once from that
temp (converted = ArrayAPIList(temp)) so the overridden append is not invoked
for each item; leave the return logic (return converted if changed else value)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c3e8b7bc-bafd-4e48-8dee-69eb6913cd3f
📒 Files selected for processing (18)
source/tests/array_api_strict/common.pysource/tests/array_api_strict/descriptor/__init__.pysource/tests/array_api_strict/descriptor/dpa1.pysource/tests/array_api_strict/descriptor/dpa2.pysource/tests/array_api_strict/descriptor/dpa3.pysource/tests/array_api_strict/descriptor/hybrid.pysource/tests/array_api_strict/descriptor/repflows.pysource/tests/array_api_strict/descriptor/repformers.pysource/tests/array_api_strict/descriptor/se_atten_v2.pysource/tests/array_api_strict/descriptor/se_e2_a.pysource/tests/array_api_strict/descriptor/se_e2_r.pysource/tests/array_api_strict/descriptor/se_t.pysource/tests/array_api_strict/descriptor/se_t_tebd.pysource/tests/array_api_strict/fitting/__init__.pysource/tests/array_api_strict/fitting/fitting.pysource/tests/array_api_strict/utils/exclude_mask.pysource/tests/array_api_strict/utils/network.pysource/tests/array_api_strict/utils/type_embed.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5528 +/- ##
==========================================
- Coverage 82.19% 82.19% -0.01%
==========================================
Files 891 891
Lines 101599 101599
Branches 4242 4242
==========================================
- Hits 83507 83506 -1
Misses 16789 16789
- Partials 1303 1304 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary:
Tests:
Summary by CodeRabbit
New Features
Refactor