fix: LibreOffice - serialize output_file_type and increase unit tests coverage - #3793
Merged
Conversation
… coverage to_dict() called default_to_dict(self) without any init parameters, so a converter built with output_file_type="pdf" serialized to empty init_parameters and came back from from_dict() with output_file_type=None. Pass the parameter through so the round trip is lossless, and update the two serialization tests that pinned the old behaviour. On top of that, cover the paths that were only reachable through the integration-marked tests, which the unit coverage badge excludes: - run_async: the whole method was at 0%, both the path and ByteStream branches, by patching create_subprocess_exec - run: the ByteStream branch, the output_file_type resolution and its ValueError, and input-type inference from the filename - _get_conversion_args: the exact soffice argv, the derived output path, and the missing-source and unwritable-outdir errors - _validate_args: every accept and reject branch The soffice stand-in derives the output path from the argv the converter built, so the tests fail if that contract changes. Fixtures move to tests/conftest.py, where test_files_path is anchored to __file__ instead of the current working directory. Unit coverage: 58% -> 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
integration:libreoffice
type:documentation
Contributor
Coverage report (libreoffice)
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Drop the separate test_converter_unit.py and merge everything into test_converter.py, split into focused classes rather than one big class: TestInit, TestSerde, TestGetConversionArgs, TestValidateArgs, TestRun, TestRunAsync, TestResolveMimeType and TestRunIntegration. This follows the review on #3794, down to the class naming, and hoists the integration and asyncio markers onto the classes instead of repeating them per method. Cover the behaviour the to_dict() fix protects, which was only asserted at the attribute level before: - run and run_async pick up output_file_type from __init__ when the argument is omitted, and the argument wins when both are set - a converter that has been through to_dict() -> from_dict() still converts without an output_file_type argument, so reverting the fix fails the test - from_dict() still accepts dictionaries serialized before output_file_type was included - both ValueError paths when output_file_type is set nowhere, and the _validate_args branch for a type outside OUTPUT_FILE_TYPE entirely Rename the `converter` fixture to `real_converter`, since next to `mock_converter` the bare name did not convey that it needs an actual LibreOffice installation, and give the remaining fixtures docstrings. Unit coverage: 93% -> 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run passes check=True to subprocess.run, so a failed conversion raises CalledProcessError. run_async discarded the exit code instead, and because every ByteStream in one call is written to the same `input` temp file, a failed conversion could return the previous source's converted bytes rather than raising. For file path sources it surfaced as a FileNotFoundError on the missing output instead. Check the exit code and raise the same CalledProcessError the sync path already raises, and document it on both methods, since run had this behaviour all along without saying so. The async soffice stand-in gains a settable returncode so the new test can make it report a failure, parametrized over a path and a ByteStream source to cover both call sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
run and run_async share their logic, so the tests duplicated every case. Collapse them into a TestConvert class parametrized over ["run", "run_async"], driven by a small _convert helper, and merge the two soffice stand-ins into one fake_soffice fixture that patches both call sites, records each argv and honours a settable returncode. This also covers run's own check=True failure path, which nothing exercised before. Parametrize the leftovers the same way: the three validation rejects and the three mime type cases each become one table, and the four integration tests become one case parametrized over sync/async and path/ByteStream sources, which still collects as four tests. Test code drops from 403 to 313 lines, 51 fewer added lines, with the same 29 unit tests and statement and branch coverage still at 100%. Async coverage now rides the shared test bodies instead of a duplicated class, so it no longer costs anything to keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ested `fake_soffice._run` raised `CalledProcessError` on a non-zero `returncode` regardless of its arguments, so `test_raises_when_soffice_exits_non_zero[*-run]` asserted the fixture's behaviour rather than the converter's: removing `check=True` from both `subprocess.run` calls left all four parametrizations green. Honour `check` the way `subprocess.run` does. The sync cases now cover the same contract the new `run_async` exit-code handling does — that a failed conversion raises instead of returning the previous source's bytes — and fail if `check=True` is ever dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
julian-risch
marked this pull request as ready for review
August 21, 2026 15:48
julian-risch
requested review from
anakin87 and
sjrl
and removed request for
a team and
anakin87
August 21, 2026 15:48
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.
Related Issues
I also found two small problems to fix so this PR does not just increase unit test coverage.
Proposed Changes:
Fixes:
to_dict()droppedoutput_file_type, so it did not survive afrom_dict()round trip. Pass it through.run_asyncdiscarded the soffice exit code, whilerunpassescheck=True. Since everyByteStreamin one call is written to the sameinputtemp file, a failed async conversion could return the previous source's converted bytes rather than raising. Now, we check the exit code and raise the sameCalledProcessErrorthe sync path already raisesTests:
run_async(was 0%), therunByteStream branch,_get_conversion_args, every_validate_argsreject branch, and theoutput_file_typeresolutionTestInit,TestSerde,TestGetConversionArgs,TestValidateArgs,TestConvert,TestResolveMimeType,TestRunIntegration.TestConvertis parametrized overrunandrun_asynctests/conftest.pyHow did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.