Early fail when output can not be created #346 #382
Conversation
Codecov Report
@@ Coverage Diff @@
## master #382 +/- ##
==========================================
+ Coverage 94.56% 94.66% +0.09%
==========================================
Files 18 18
Lines 2115 2154 +39
Branches 370 373 +3
==========================================
+ Hits 2000 2039 +39
+ Misses 55 54 -1
- Partials 60 61 +1
Continue to review full report at Codecov.
|
b433336 to
174ca99
Compare
There was a problem hiding this comment.
Thanks for this, this is great as usual! But I think I've stumbled upon a larger bug in gcovr's config file handling 🤔, and I have opinions on error message phrasing.
None of those are blockers for this PR, so just let me know if you'd want to merge directly or use this space for further discussion first.
I'm not enthusiastic about the low code coverage for this patch, but I don't see how it could be tested reliably, so this is fine.
|
I've tried to add a test with a temporary file and using chmod but it didn't work. Maybe this is because I'm running the test in Docker. |
1a51fe7 to
dcf1077
Compare
|
Testing permissions is going to be somewhat nonportable, at least when Windows is involved. Otherwise, But given the difficulties involved, it's OK if the test is skipped. Gcovr isn't that critical, so some amount of testing in “production” is fine :) |
|
What about to change to the python way. Instead of doing all the checks for the permission try to write the file and unlink it? |
|
I'm trying to solve the problem found within the new config test. Jinja fails if the template is in a sub directory, in docker it works. |
ef29e79 to
cefdfe2
Compare
The FileSystemLoader does not support templates in sub directories on Windows (appveyor).
cefdfe2 to
e4a058b
Compare
Well, there are tradeoffs. WIth the Python Way you presumably mean “it's better to ask forgiveness than permission”. That's generally sensible, but is also what we were doing previously: just try to create the report, and if it fails, it fails. Here, our goal is not to create the report, but to quickly alert the users about problems.
This differs in the outcome if gcovr crashes between the test and writing the report. I think there's a 10% chance of someone complaining about this in the future, but the simple destructive check is not incorrect.
Thanks for the fix! Jinja is geared towards a web context where there are different security concerns than we have, so apparently it takes some extra work to load arbitrary files. |
|
I've merged as-is. The tradeoff between different approaches relates to the concept of exception safety e.g. in C++ programming: things are safe if we end up in a valid state (and deleting files is valid), but under a stronger exception safety guarantee changes would be atomic: the attempted action either fails or produces an effect. Since gcovr isn't a concurrent database system and since we don't make particular guarantees, deleting files during the check is fine. |
The options for output files are checked if the file is writable and the parent directory is writable.
Checks extended woth output files in non existing folders. Don't now how to check a write protected folder.