📝 chore: Add mixed-input regression coverage to generate config tests#607
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Greptile SummaryThis PR adds
Confidence Score: 4/5Test-only change; the assertions match the actual migration logic and will not regress production behavior. All assertions are consistent with the _migrate_legacy_structured_generation_fields implementation. The two duplicate/misclassified tests are noise but not wrong. The only real gap is missing a pytest.raises test for a mixed-input pair that produces an invalid merged config, leaving one branch of the migration+validation interaction untested. tests/config/test_generate.py — the two non-mixed-input tests and the absent validation-failure test are worth a second look. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.unit | ||
| class TestMixedInputMigration: | ||
| """Regression coverage for mixed-input: legacy flat keys coexist with nested structured_generation dict. | ||
|
|
||
| The migration contract: when both forms are present, legacy flat keys win as explicit overrides. | ||
| Un-overridden nested dict fields are preserved. | ||
| """ | ||
|
|
||
| def test_legacy_backend_overrides_nested_dict_backend(self) -> None: | ||
| """A legacy flat backend key beats the nested dict backend when both are supplied.""" | ||
| params = GenerateParameters.model_validate( | ||
| { | ||
| "structured_generation": {"enabled": True, "backend": "outlines"}, | ||
| "structured_generation_backend": "xgrammar", | ||
| } | ||
| ) | ||
| assert params.structured_generation.backend == "xgrammar" | ||
| assert params.structured_generation.enabled is True # nested value preserved | ||
|
|
||
| def test_legacy_enabled_overrides_nested_dict_enabled(self) -> None: | ||
| """A legacy use_structured_generation key beats the nested dict enabled field.""" | ||
| params = GenerateParameters.model_validate( | ||
| { | ||
| "use_structured_generation": True, | ||
| "structured_generation": {"enabled": False, "schema_method": "json_schema"}, | ||
| } | ||
| ) | ||
| assert params.structured_generation.enabled is True | ||
| assert params.structured_generation.schema_method == "json_schema" # preserved | ||
|
|
||
| def test_legacy_schema_method_overrides_nested_dict_schema_method(self) -> None: | ||
| """A legacy schema_method key beats the nested dict schema_method field.""" | ||
| params = GenerateParameters.model_validate( | ||
| { | ||
| "structured_generation_schema_method": "regex", | ||
| "structured_generation": {"enabled": True, "schema_method": "json_schema"}, | ||
| } | ||
| ) | ||
| assert params.structured_generation.schema_method == "regex" | ||
| assert params.structured_generation.enabled is True # preserved | ||
|
|
||
| def test_from_params_legacy_backend_overrides_generation_section_backend(self) -> None: | ||
| """from_params: a top-level legacy backend key overrides the backend inside generation={}.""" | ||
| params = SafeSynthesizerParameters.from_params( | ||
| generation={"structured_generation": {"enabled": True, "backend": "outlines"}}, | ||
| structured_generation_backend="xgrammar", | ||
| ) | ||
| assert params.generation.structured_generation.backend == "xgrammar" | ||
| assert params.generation.structured_generation.enabled is True # preserved | ||
|
|
||
| def test_from_params_legacy_enabled_overrides_structured_generation_kwarg(self) -> None: | ||
| """from_params: a top-level legacy use_structured_generation overrides structured_generation kwarg.""" | ||
| params = SafeSynthesizerParameters.from_params( | ||
| structured_generation={"enabled": False, "schema_method": "json_schema"}, | ||
| use_structured_generation=True, | ||
| ) | ||
| assert params.generation.structured_generation.enabled is True | ||
| assert params.generation.structured_generation.schema_method == "json_schema" # preserved |
There was a problem hiding this comment.
Every mixed-input test here asserts a valid final state. The migration contract is that legacy keys act as overrides, so a caller who passes structured_generation={"schema_method": "structural_tag", "backend": "xgrammar"} alongside a legacy structured_generation_backend="outlines" override would end up with schema_method="structural_tag" + backend="outlines" — an incompatible pair that the _validate_structural_tag_backend validator must reject. Without a pytest.raises test for this path there is no regression coverage for the case where the merged result is invalid, and a future change to the merge order or validator could silently break it.
mckornfield
changed the title
2bd332c
into
move-structured-gen-parameters/mck
…607) Code changes was requested by @binaryaaron. * #595 (comment) The following files were modified: * `tests/config/test_generate.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: mkornfield <mkornfield@nvidia.com>
2 participants
Code changes was requested by @binaryaaron.
The following files were modified:
tests/config/test_generate.py