-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a test for permission denied test case #448
Conversation
This change adds a test checking expected behavior when access to a symbolization source file is denied. Signed-off-by: Daniel Müller <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #448 +/- ##
==========================================
- Coverage 92.30% 92.26% -0.05%
==========================================
Files 41 41
Lines 6397 6347 -50
==========================================
- Hits 5905 5856 -49
+ Misses 492 491 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like too much effort just to test this condition, especially taking into account all the "Ugh" remarks ;)
If you absolutely want to have a test for this specific conditions, can you use some root-only file as a source. E.g., /proc/kcore (assuming it's Linux and kcore is present, of course), or /etc/sudoers (seems to not allow reading for non-root).
It does, but the effort has been invested by now. The test is entirely optional and won't run by default (though it will run in CI, but that's a pretty stable environment overall and we can always disable it if it turns out to be flaky).
The problem is more that all tests are running as root so we will always have access to these files. That's why the fork dance is necessary, so that we can run as different user. The main reason for checking this condition was that I wanted to test the behavior that Salvatore described. There is admittedly no coverage increase, but I'd say it's still valuable to have and risk and maintenance burden seem low to me. Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not. We can always delete it if it causes problems in the future, of course.
A bunch of the nastiness evaporates once we move this test into a separate binary. |
This change adds a test checking expected behavior when access to a symbolization source file is denied.