Add force_exec_validation option to catch runtime errors across Python versions#2719
Add force_exec_validation option to catch runtime errors across Python versions#2719
Conversation
|
Warning Rate limit exceeded@koxudaxi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis change introduces runtime execution validation to the test suite by exposing the current Python version through a new constant and pytest fixture, and by extending validation functions with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
f604158 to
64d29f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/main/test_exec_validation.py (2)
131-152: LGTM!These helper tests correctly validate
get_current_version_argsbehavior and theCURRENT_PYTHON_VERSIONformat. Note: The pipeline errors for ANN001 on lines 134, 139, 149 appear to be incorrect since these methods only haveselfas a parameter and don't useoutput_file.However, consider adding return type annotations for consistency:
- def test_get_current_version_args_basic(self): + def test_get_current_version_args_basic(self) -> None:
29-30: Consider suppressing PLR6301 for test classes.The
PLR6301linting errors suggest converting test methods to static methods or functions. However, grouping tests in classes is a common pytest pattern for organization. You can suppress this rule for test files.Options:
- Add
# noqa: PLR6301to each method- Configure ruff/pylint to ignore PLR6301 in test files
- Convert classes to standalone functions (less organized but lint-clean)
The class-based organization provides good grouping by schema type, so option 2 is recommended.
Also applies to: 40-41, 51-52, 66-67, 77-78, 88-89, 107-112, 134-134, 139-139, 149-149
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/main/conftest.py(9 hunks)tests/main/test_exec_validation.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
tests/main/test_exec_validation.py
[error] 30-30: PLR6301 Method test_openapi_api_exec_current_version could be a function, class method, or static method
[error] 30-30: ANN001 Missing type annotation for function argument output_file
[error] 41-41: PLR6301 Method test_openapi_with_refs_exec_current_version could be a function, class method, or static method
[error] 41-41: ANN001 Missing type annotation for function argument output_file
[error] 52-52: PLR6301 Method test_openapi_allof_exec_current_version could be a function, class method, or static method
[error] 52-52: ANN001 Missing type annotation for function argument output_file
[error] 67-67: PLR6301 Method test_jsonschema_person_exec_current_version could be a function, class method, or static method
[error] 67-67: ANN001 Missing type annotation for function argument output_file
[error] 78-78: PLR6301 Method test_jsonschema_nested_array_exec_current_version could be a function, class method, or static method
[error] 78-78: ANN001 Missing type annotation for function argument output_file
[error] 89-89: PLR6301 Method test_jsonschema_circular_reference_exec_current_version could be a function, class method, or static method
[error] 89-89: ANN001 Missing type annotation for function argument output_file
[error] 112-112: PLR6301 Method test_force_exec_with_different_target_version could be a function, class method, or static method
[error] 112-112: ANN001 Missing type annotation for function argument output_file
[error] 134-134: PLR6301 Method test_get_current_version_args_basic could be a function, class method, or static method
[error] 134-134: ANN001 Missing type annotation for function argument output_file
[error] 139-139: PLR6301 Method test_get_current_version_args_with_extra could be a function, class method, or static method
[error] 139-139: ANN001 Missing type annotation for function argument output_file
[error] 149-149: PLR6301 Method test_current_python_version_format could be a function, class method, or static method
[error] 149-149: ANN001 Missing type annotation for function argument output_file
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: 3.11 on Windows
- GitHub Check: 3.10 on Ubuntu
- GitHub Check: 3.10 on Windows
- GitHub Check: 3.12 on Windows
- GitHub Check: py312-black23 on Ubuntu
- GitHub Check: 3.14 on Windows
- GitHub Check: Analyze (python)
- GitHub Check: benchmarks
🔇 Additional comments (8)
tests/main/conftest.py (7)
56-57: LGTM!Clean implementation of the version constant using
sys.version_info. The docstring provides a helpful example.
108-116: LGTM!The fixture correctly exposes
CURRENT_PYTHON_VERSIONand includes helpful documentation about its purpose for enabling exec() validation.
118-131: LGTM!The helper function is well-designed with clear documentation and a practical example. Using
*extra_argswith unpacking provides a clean API.
419-438: LGTM!The logic correctly handles the
force_execparameter:
- Returns early if pydantic version mismatches model type
- Returns early if no target version specified
- When
force_exec=False, skips if target ≠ runtime (original behavior)- When
force_exec=True, allows exec to proceed (target ≤ runtime is ensured by_should_skip_compile)The docstring clearly explains the interaction with target/runtime version checks.
441-459: LGTM!The updated
_validate_output_filesfunction properly threads theforce_exec_validationparameter through to_should_skip_exec. The docstring accurately describes when the parameter has no effect (target > runtime).
254-254: LGTM!The
force_exec_validationparameter is properly integrated intorun_main_and_assert:
- Added to function signature with sensible default (
False)- Well-documented in the docstring explaining its behavior and limitations
- Correctly passed to
_validate_output_filesAlso applies to: 291-297, 388-388
531-531: LGTM!Consistent propagation of
force_exec_validationthroughrun_main_url_and_assert, matching the pattern established inrun_main_and_assert.Also applies to: 543-544, 551-551
tests/main/test_exec_validation.py (1)
1-7: LGTM!Clear module-level docstring explaining the purpose of these tests and referencing the relevant skip logic in conftest.py.
64d29f4 to
7d56e30
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2719 +/- ##
=======================================
Coverage 99.34% 99.35%
=======================================
Files 82 83 +1
Lines 11615 11708 +93
Branches 1406 1410 +4
=======================================
+ Hits 11539 11632 +93
Misses 45 45
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #2719 will not alter performanceComparing Summary
Footnotes
|
7d56e30 to
2407f23
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.