Skip to content
This repository was archived by the owner on Dec 28, 2025. It is now read-only.

Conversation

@handrews
Copy link
Contributor

For incomprehensible reasons apparently having to do with backwards compatibility, the filename parameter to OSError is not part of the args member, so the exception handling code that uses args to convert to a CatalogError was dropping the rather important filename from the FileNotFound error.

Note that this PR shares the fixture commit with PR #71

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.52 🎉

Comparison is base (f5df6c4) 91.56% compared to head (dadca5e) 92.08%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   91.56%   92.08%   +0.52%     
==========================================
  Files          21       21              
  Lines        2003     2010       +7     
  Branches      427      429       +2     
==========================================
+ Hits         1834     1851      +17     
+ Misses        110      101       -9     
+ Partials       59       58       -1     
Impacted Files Coverage Δ
jschon/catalog/__init__.py 98.13% <100.00%> (+6.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@handrews
Copy link
Contributor Author

The only thing I can think to do for the coverage complaint is to mock out open() (which I'd have to research as I haven't done such a thing in about 10 years) and make it raise a TimeoutError or something similar that doesn't set the filename.

I can do that if you want, or slap a # pragma: no cover on it, or just leave it as-is.

@handrews
Copy link
Contributor Author

I've now added a test case that mocks a filename-less IOError so the test coverage check should be happy now 😁

BTW I am so glad that your unit tests are not the style that mocks literally everything but a single function/method under test. Mocks are great for situations like this case, but I've seen people overdo it to the point that their tests are like 80% mock configuration that has to change at the slightest refactoring.

@handrews
Copy link
Contributor Author

@marksparkza this short PR shares a test fixture commit with #71, so you probably want to merge this before doing anything complicated with #71's branch. I can rebase #71 if needed.

handrews added 2 commits April 9, 2023 10:13
For incomprehensible reasons apparently having to do with
backwards compatibility, the filename parameter to OSError
is not part of the 'args' member, so the exception handling
code that uses 'args' to convert to a CatalogError was dropping
the rather important filename from the FileNotFound error.
Mock open() to simulate a non-file IOError.
@marksparkza marksparkza merged commit 3c3b770 into marksparkza:main Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants