Tests refactor - #263
Open
kozlov721 wants to merge 73 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR consolidates CI workflows, adds Docker development support, introduces layout-aware preprocessing, updates conversion behavior, replaces legacy package tests with host-side unit tests, and adds broad cross-platform conversion, precision, archive, calibration, evaluation, and type coverage. ChangesCI and development environment
Runtime conversion behavior
Conversion and evaluation coverage
Host-side unit tests
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
kozlov721
force-pushed
the
tests/improvement
branch
from
July 25, 2026 19:26
f295733 to
c0f6bf2
Compare
kozlov721
force-pushed
the
tests/improvement
branch
from
July 27, 2026 09:57
7aee76c to
81b34fe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelconverter/packages/hailo/exporter.py (1)
246-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate normalization vector lengths before emitting the Hailo script.
The config parse path now accepts scalar normalization lists as
[v, v, v], so non-3-channel inputs can still be serialized with legacy 3-element means/scales. Reject mismatchedmean_valuesandscale_valuesbefore appending thenormalization_...command.🤖 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 `@modelconverter/packages/hailo/exporter.py` around lines 246 - 251, Validate mean_values and scale_values in the normalization emission flow before appending the normalization command: each vector must match values_len, rejecting mismatched lengths, including legacy 3-element values for non-3-channel inputs. Preserve the existing defaults for missing values and only emit normalization after both vectors pass validation.
🧹 Nitpick comments (2)
modelconverter/packages/base_exporter.py (1)
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDo not exclude reachable conversion paths from coverage.
Line 133 excludes the entire
simplify_onnxbody, while Lines 204 and 240 hide normal cleanup and explicit validation behavior. Add focused tests/mocks and reservepragma: no coverfor genuinely untestable external failures.Suggested change
- def simplify_onnx(self) -> Path: # pragma: no cover + def simplify_onnx(self) -> Path:Also applies to: 204-204, 240-240
🤖 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 `@modelconverter/packages/base_exporter.py` at line 133, Remove the `pragma: no cover` exclusions from `simplify_onnx` and the cleanup and validation branches at the referenced locations, then add focused tests with mocks that exercise those reachable conversion paths. Keep coverage exclusions only for genuinely untestable external failure handling.tests/unit/test_evaluation.py (1)
37-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the source-floor failure branch of
assert_quality.Only the max-drop failure path is exercised; the floor-check branch (
reference[metric] < floor) is never triggered by a failing case, leaving that branch uncovered.Suggested addition
+def test_assert_quality_checks_source_floor() -> None: + with pytest.raises(AssertionError, match="below floor"): + assert_quality( + {"AP": 0.10}, + {"AP": 0.10}, + floors={"AP": 0.39}, + max_drops={"AP": 0.03}, + case_id="test", + platform="rvc4", + )🤖 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 `@tests/unit/test_evaluation.py` around lines 37 - 55, Add a failing test case to test_assert_quality_checks_source_floor_and_drop that makes reference[metric] fall below its configured floor while avoiding the max-drop condition, and assert that assert_quality raises AssertionError with the expected floor-related message. Keep the existing max-drop coverage 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.
Inline comments:
In `@modelconverter/packages/base_exporter.py`:
- Around line 252-255: Update the calibration handling around the is_raw_input
condition so raw inputs retain an array/raw-compatible calibration configuration
when random calibration writes .npy files; do not replace it with
ImageCalibrationConfig unless the data remains image-backed and compatible with
read_img_dir. Preserve the existing image calibration behavior for non-raw
inputs.
In `@tests/conversion/test_model_evaluation.py`:
- Around line 129-172: Update the return type annotation of
_lazy_eval_components to declare all five returned tuple elements, including the
callable corresponding to get_metric_ctx, while preserving the existing parser,
metrics, and mapping callable types.
---
Outside diff comments:
In `@modelconverter/packages/hailo/exporter.py`:
- Around line 246-251: Validate mean_values and scale_values in the
normalization emission flow before appending the normalization command: each
vector must match values_len, rejecting mismatched lengths, including legacy
3-element values for non-3-channel inputs. Preserve the existing defaults for
missing values and only emit normalization after both vectors pass validation.
---
Nitpick comments:
In `@modelconverter/packages/base_exporter.py`:
- Line 133: Remove the `pragma: no cover` exclusions from `simplify_onnx` and
the cleanup and validation branches at the referenced locations, then add
focused tests with mocks that exercise those reachable conversion paths. Keep
coverage exclusions only for genuinely untestable external failure handling.
In `@tests/unit/test_evaluation.py`:
- Around line 37-55: Add a failing test case to
test_assert_quality_checks_source_floor_and_drop that makes reference[metric]
fall below its configured floor while avoiding the max-drop condition, and
assert that assert_quality raises AssertionError with the expected floor-related
message. Keep the existing max-drop coverage unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66ee9eb5-289c-462d-bffa-22640ee4478c
📒 Files selected for processing (16)
.github/workflows/ci.yamldocker/rvc2/Dockerfiledocker/rvc4/Dockerfilemodelconverter/packages/base_exporter.pymodelconverter/packages/hailo/exporter.pymodelconverter/packages/rvc2/exporter.pymodelconverter/packages/rvc4/inferer.pyrequirements-eval.txttests/conversion/test_model_evaluation.pytests/conversion/test_precision.pytests/conversion/test_toy_integration.pytests/helpers/evaluation.pytests/helpers/onnx_reference.pytests/unit/test_environ.pytests/unit/test_evaluation.pytests/unit/test_random_calibration.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/unit/test_random_calibration.py
- modelconverter/packages/rvc2/exporter.py
- tests/conversion/test_precision.py
- .github/workflows/ci.yaml
- tests/unit/test_environ.py
- tests/helpers/onnx_reference.py
- docker/rvc4/Dockerfile
- tests/conversion/test_toy_integration.py
kozlov721
force-pushed
the
tests/improvement
branch
from
July 28, 2026 00:42
1854081 to
9224bc2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@tests/test_benchmark/test_benchmark_regression.py`:
- Around line 215-216: Update test_benchmark_fps to keep the FPS validation
independent of Influx configuration: remove the mandatory influx_token
assertion, allow the conftest-provided optional token, and guard telemetry
persistence so it runs only when influx_token is available while retaining the
FPS check regardless.
- Line 225: Update the expected_fps validation in the benchmark regression test
to accept valid JSON numeric values represented as integers by normalizing
expected_fps with float(...) before comparison, rather than requiring
isinstance(expected_fps, float). Preserve rejection of non-numeric values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d7bbde0-5386-458b-b957-7c9d9ef29afe
📒 Files selected for processing (12)
.github/workflows/ci.yamlmodelconverter/utils/nn_archive.pytests/conversion/test_conversion.pytests/conversion/test_model_evaluation.pytests/helpers/onnx_reference.pytests/test_benchmark/conftest.pytests/test_benchmark/test_benchmark_regression.pytests/unit/test_cli_utils.pytests/unit/test_config.pytests/unit/test_nn_archive.pytests/unit/test_target_versions.pytests/unit/test_types.py
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/unit/test_target_versions.py
- modelconverter/utils/nn_archive.py
- tests/helpers/onnx_reference.py
- .github/workflows/ci.yaml
- tests/conversion/test_conversion.py
- tests/conversion/test_model_evaluation.py
- tests/unit/test_cli_utils.py
- tests/unit/test_config.py
- tests/unit/test_types.py
- tests/unit/test_nn_archive.py
kozlov721
force-pushed
the
tests/improvement
branch
from
July 28, 2026 03:48
29204ed to
f632f62
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modelconverter/utils/docker_utils.py (1)
81-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not leave
HUBAI_API_KEYin the generated compose file.This key is serialized into a temporary YAML file by
docker_exec(Line 466 onward), but that file is created withdelete=Falseand never unlinked. Each run can therefore leave the API key in plaintext on disk. Use a secret/env-file mechanism and remove the compose file in afinallyblock.🤖 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 `@modelconverter/utils/docker_utils.py` around lines 81 - 86, Update docker_exec and the environment construction around the HUBAI_API_KEY entry so the key is passed through a Docker secret or env-file mechanism rather than serialized into the generated compose YAML. Ensure the temporary compose file created with delete=False is always removed in a finally block, including when execution fails, while preserving existing container environment behavior.pyproject.toml (1)
182-182: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the
assertexclusion word-bounded.In
[tool.coverage.report],"assert"is a regex that can match identifiers like_assert_correct, causing helper statements to be excluded even though coverage sources includetests. Use\bassert\bto exclude only assertion statements.🤖 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 `@pyproject.toml` at line 182, Update the "assert" exclusion entry in the [tool.coverage.report] configuration to use a word-bounded regular expression, \bassert\b, so identifiers containing assert are not excluded while assertion statements remain excluded.tests/conversion/test_model_evaluation.py (1)
234-297: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnsure calibration images come from the same parsed dataset samples used for evaluation.
CALIBRATION_SPECloads only"train", whilecoco_sampleiteratesview=["train", "val", "test"]; ifval/testsamples exist or are ordered differently,zip(coco_sample, images, strict=True)can pair wrong labels with the exported predictions. Use the same view list in both places, use only one source collection for both calibration materialization and metric labels, or assert/document that the data istrain-only.🤖 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 `@tests/conversion/test_model_evaluation.py` around lines 234 - 297, Update the evaluation setup around CALIBRATION_SPEC, coco_sample, and _calibration_images so calibration images and metric labels come from the same parsed dataset view and collection, preserving sample order for the strict zip evaluation loop. Use the shared train/val/test view list consistently, or explicitly constrain both paths to train-only if that is the intended contract.
🧹 Nitpick comments (1)
tests/conversion/test_model_evaluation.py (1)
276-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid relying on
LuxonisLoader’s private_classesattribute.
coco_sample._classes[""]reads an underscore-prefixed attribute, which is not a stable public API and can break acrossluxonis_mlreleases. Add a small test helper that receives the class mapping as a public argument instead of reading loader internals.🤖 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 `@tests/conversion/test_model_evaluation.py` around lines 276 - 278, Replace the direct coco_sample._classes access in the ldf_class_map setup with a small test helper that accepts the class mapping through a public argument. Update the relevant test calls to pass that mapping explicitly, preserving the existing name-to-index conversion without depending on LuxonisLoader internals.
🤖 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.
Inline comments:
In `@modelconverter/packages/rvc2/inferer.py`:
- Around line 17-24: Update the input-processing loop so self.layout is
initialized or refreshed for every four-dimensional input, regardless of whether
self.in_shapes already matches the runtime shape. Keep the shape-cache update
conditional in the existing guard, but perform the NCHW/NHWC inference
independently before loading the network in exec_net.
In `@tests/conftest.py`:
- Around line 22-24: Remove the global suppress_health_check setting from the
modelconverter profile in tests/conft.py, and apply
HealthCheck.function_scoped_fixture suppression only to the specific tests or
modules that intentionally reuse tmp_path across Hypothesis examples. Preserve
the suppression for those affected cases while allowing fixture-isolation checks
elsewhere.
---
Outside diff comments:
In `@modelconverter/utils/docker_utils.py`:
- Around line 81-86: Update docker_exec and the environment construction around
the HUBAI_API_KEY entry so the key is passed through a Docker secret or env-file
mechanism rather than serialized into the generated compose YAML. Ensure the
temporary compose file created with delete=False is always removed in a finally
block, including when execution fails, while preserving existing container
environment behavior.
In `@pyproject.toml`:
- Line 182: Update the "assert" exclusion entry in the [tool.coverage.report]
configuration to use a word-bounded regular expression, \bassert\b, so
identifiers containing assert are not excluded while assertion statements remain
excluded.
In `@tests/conversion/test_model_evaluation.py`:
- Around line 234-297: Update the evaluation setup around CALIBRATION_SPEC,
coco_sample, and _calibration_images so calibration images and metric labels
come from the same parsed dataset view and collection, preserving sample order
for the strict zip evaluation loop. Use the shared train/val/test view list
consistently, or explicitly constrain both paths to train-only if that is the
intended contract.
---
Nitpick comments:
In `@tests/conversion/test_model_evaluation.py`:
- Around line 276-278: Replace the direct coco_sample._classes access in the
ldf_class_map setup with a small test helper that accepts the class mapping
through a public argument. Update the relevant test calls to pass that mapping
explicitly, preserving the existing name-to-index conversion without depending
on LuxonisLoader internals.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b4872ce-811c-467c-beae-8852f43063c0
📒 Files selected for processing (44)
.github/workflows/ci.yamlmodelconverter/packages/hailo/exporter.pymodelconverter/packages/rvc2/exporter.pymodelconverter/packages/rvc2/inferer.pymodelconverter/utils/docker_utils.pypyproject.tomlrequirements-dev.txttests/conftest.pytests/conversion/test_conversion.pytests/conversion/test_conversion_variants.pytests/conversion/test_external_data.pytests/conversion/test_hailo_compile.pytests/conversion/test_model_evaluation.pytests/conversion/test_precision.pytests/conversion/test_rvc4_encodings.pytests/conversion/test_toy_integration.pytests/conversion/test_toy_multistage.pytests/conversion/test_toy_precision.pytests/conversion/test_toy_tflite.pytests/helpers/archive_factory.pytests/helpers/conversion.pytests/helpers/evaluation.pytests/helpers/onnx_factory.pytests/helpers/onnx_reference.pytests/helpers/platforms.pytests/helpers/precision.pytests/helpers/strategies.pytests/helpers/target_options.pytests/helpers/tflite_factory.pytests/unit/test_calibration_data.pytests/unit/test_cli_utils.pytests/unit/test_config.pytests/unit/test_conversion_helpers.pytests/unit/test_encodings.pytests/unit/test_filesystem_utils.pytests/unit/test_general.pytests/unit/test_getters.pytests/unit/test_image.pytests/unit/test_layout.pytests/unit/test_metadata.pytests/unit/test_nn_archive.pytests/unit/test_random_calibration.pytests/unit/test_subprocess.pytests/unit/test_types.py
💤 Files with no reviewable changes (2)
- tests/helpers/archive_factory.py
- tests/helpers/tflite_factory.py
🚧 Files skipped from review as they are similar to previous changes (5)
- modelconverter/packages/hailo/exporter.py
- tests/unit/test_getters.py
- tests/unit/test_filesystem_utils.py
- tests/unit/test_cli_utils.py
- tests/unit/test_nn_archive.py
`RVC2Inferer.setup` skipped the layout assignment whenever the configured shape already matched the IR's, so an NHWC IR whose config carried no layout reached `infer` as channels-first and `read_image` transposed it. The shape and the layout now both come from the IR, with a host-side regression test driving `setup` against a stubbed OpenVINO. `suppress_health_check=[function_scoped_fixture]` was set on the whole `modelconverter` Hypothesis profile, silencing the check for every property test. Only seven of them reuse `work_dir`/`monkeypatch` on purpose, so they now opt in individually via `reuses_function_fixtures`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Previous tests were insufficient. This PR completely refactors the tests, adding coverage reporting and simplified CI.
Specification
tests/conversiontests for more reliable testing of model conversionsmodelconverter--devflag now also installs thedevrequirements to the image and mountstests/from PWDmodelconverter shell rvc4 --dev -c 'pytest -x'Dependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
Summary by CodeRabbit
New Features
Bug Fixes
Refactor