Repository navigation
Conversation
|
@kosmylo check . |
|
Thanks for the contribution. This is a useful input-validation fix, and applying the same checks in both entry points is appropriate. I found no defect in the new numeric guards during the checks performed. Before merging, include the new regression module in the existing pytest CI workflow and align the documented test command. Keep the separate Level 2 unittest job scoped to its own module rather than discovering pytest files. Please also add successful loader-to-validator cases for integers, floats, and zero. A brief clarification in the Level 1 Solution File Contract about finite numeric values and rejection of booleans would make the rule explicit. |
| import json | ||
| from pathlib import Path | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
These are pytest tests, but the shared workflow now on main runs only tests/test_benchmark_utils_contract.py. This new module would therefore remain outside that job.
Please update the existing workflow to include this file, either by naming both Level 1 test modules or by running python -m pytest -q tests. Align the contributor test command and obtain a passing run that includes these 12 cases. Do not add another workflow.
The separate Level 2 unittest discover -s tests command does not execute parametrized pytest functions. Its current install step also does not explicitly install pytest. Keep that job scoped to test_anytime_risk.py, or use an appropriately configured pytest runner. That cross-workflow issue is already present on current main, not a defect in this numeric-validation change.
| def test_load_solution_rejects_invalid_numeric_values( | ||
| tmp_path: Path, value: object, message: str | ||
| ) -> None: | ||
| path = tmp_path / "solution.json" | ||
| path.write_text( | ||
| json.dumps({"actions": [{"id": "tap_T1_up", "value": value}]}), | ||
| encoding="utf-8", | ||
| ) | ||
|
|
||
| with pytest.raises(ValueError, match=message): | ||
| load_solution(path) |
There was a problem hiding this comment.
Please add successful load_solution() cases alongside these rejection cases. The new module only exercises invalid values, and the existing contract suite does not test a successful solution-file load.
A small parametrized test using values such as 0, 1, and 1.0 can load the JSON, pass the result to validate_action_values(), and assert the expected float values and zero defaults for omitted actions. Include a permitted negative or fractional value with an appropriate action fixture.
This protects the distinction between rejecting booleans and continuing to accept ordinary numeric values. The current implementation passes these cases in the checks performed, so this is a regression-coverage improvement rather than a reported runtime bug.
6cb8e1a to
60fa436
Compare
|
Thanks for the updates. The CI integration, successful-input tests, and numeric-value documentation are addressed. The configured CI jobs pass with 33 pytest cases and 14 Level 2 tests. Please finish these documentation updates:
|
Summary
Reject True and False as numeric action values.
Reject NaN, positive and negative infinity, and integers that overflow during float conversion.
Apply validation consistently in load_solution() and validate_action_values().
Add regression tests for both validation paths, including oversized integers.
Testing
Added regression coverage for booleans, non-finite values, and oversized integers.
GitHub Actions results are pending verification for this new PR.