You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Raise MissingFileSystem instead of AttributeError when diffing reports without source files - #201
Diffing two reports that were constructed without a filesystem crashes with
AttributeError: 'NoneType' object has no attribute 'has_file'
Cobertura.__init__ leaves self.filesystem as None when no source is given, and CoberturaDiff.file_source() reaches straight past the public accessor into self.coberturaN.filesystem.has_file(filename).
Why the existing error path was unreachable
Cobertura already has a dedicated exception and a helpful message for exactly this situation — Cobertura.MissingFileSystem, raised by _raise_MissingFileSystem() and used by file_source():
...you need to provide the source code directory or an archive...
CoberturaDiff.file_source() simply bypassed it by touching .filesystem directly, so the user got a bare AttributeError from deep inside the library instead of the message the author had already written.
Fix
Add Cobertura._filesystem_has_file(), which raises MissingFileSystem when there is no filesystem and otherwise delegates to it, and call it from the two places in CoberturaDiff.file_source() that reached into .filesystem directly. No new semantics: this only routes an existing crash into the existing error path.
Tests
test_diff_file_source_without_filesystem in tests/test_cobertura_diff.py, parametrised over a file present in both reports and one present in only one of them. It fails on master with the AttributeError above and passes with the change.
The test builds Cobertura directly rather than through tests/utils.py::make_cobertura, because that helper always passes a filesystem — going through it, the test would be green either way.
Also in this PR
CHANGES.md entry under ## Unreleased.
README.md: the TextReporterDelta example constructs both Cobertura objects without a filesystem and then calls delta.generate(), which is precisely the crash this PR fixes — so the example as written could not have worked. Updated it to pass filesystem_factory('source1'/'source2'), matching the pycobertura diff invocation shown just above it.
black leaves every file in this PR unchanged.
AI-assisted: I used Claude to help trace the call path and to draft the patch and the test. I reviewed every line, ran the new test against unpatched master first to confirm it fails without the fix, and ran the suite locally; the analysis and the runs are mine.
Ping — this has been open since 6 August with no review yet.
Diffing two reports built without a filesystem crashes with AttributeError: 'NoneType' object has no attribute 'has_file', because CoberturaDiff.file_source() reaches past the public accessor straight into self.coberturaN.filesystem. Cobertura already has a dedicated MissingFileSystem exception with a helpful message for exactly this case — it was simply being bypassed. The change adds Cobertura._filesystem_has_file() and calls it from the two places that reached in directly, so no new semantics, just an existing crash routed into the existing error path.
build is green on 3.9 through 3.13 for 8e84e9c. The new test fails on master with the AttributeError and passes with the change.
Happy to close it if you would rather handle #171 another way.
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
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.
Fixes #171.
Problem
Diffing two reports that were constructed without a filesystem crashes with
Cobertura.__init__leavesself.filesystemasNonewhen no source is given, andCoberturaDiff.file_source()reaches straight past the public accessor intoself.coberturaN.filesystem.has_file(filename).Why the existing error path was unreachable
Coberturaalready has a dedicated exception and a helpful message for exactly this situation —Cobertura.MissingFileSystem, raised by_raise_MissingFileSystem()and used byfile_source():CoberturaDiff.file_source()simply bypassed it by touching.filesystemdirectly, so the user got a bareAttributeErrorfrom deep inside the library instead of the message the author had already written.Fix
Add
Cobertura._filesystem_has_file(), which raisesMissingFileSystemwhen there is no filesystem and otherwise delegates to it, and call it from the two places inCoberturaDiff.file_source()that reached into.filesystemdirectly. No new semantics: this only routes an existing crash into the existing error path.Tests
test_diff_file_source_without_filesystemintests/test_cobertura_diff.py, parametrised over a file present in both reports and one present in only one of them. It fails onmasterwith theAttributeErrorabove and passes with the change.The test builds
Coberturadirectly rather than throughtests/utils.py::make_cobertura, because that helper always passes a filesystem — going through it, the test would be green either way.Also in this PR
CHANGES.mdentry under## Unreleased.README.md: theTextReporterDeltaexample constructs bothCoberturaobjects without a filesystem and then callsdelta.generate(), which is precisely the crash this PR fixes — so the example as written could not have worked. Updated it to passfilesystem_factory('source1'/'source2'), matching thepycobertura diffinvocation shown just above it.blackleaves every file in this PR unchanged.AI-assisted: I used Claude to help trace the call path and to draft the patch and the test. I reviewed every line, ran the new test against unpatched
masterfirst to confirm it fails without the fix, and ran the suite locally; the analysis and the runs are mine.