many: replace rootPath with dirs.GlobalRootDir - #17007
Merged
sergiocazzolato merged 4 commits intoMay 20, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17007 +/- ##
==========================================
+ Coverage 77.92% 79.07% +1.15%
==========================================
Files 1355 1373 +18
Lines 188176 191439 +3263
Branches 2465 2465
==========================================
+ Hits 146628 151386 +4758
+ Misses 32794 30945 -1849
- Partials 8754 9108 +354
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Mon May 18 16:57:15 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
olivercalder
reviewed
May 1, 2026
olivercalder
left a comment
olivercalder
left a comment
Member
There was a problem hiding this comment.
Thanks! I think the separate mock function should be removed, and the non-trivial part is making sure the dependency on dirs is fine (which I'm almost certain it is).
| old := rootPath | ||
| rootPath = path | ||
| old := dirs.GlobalRootDir | ||
| dirs.GlobalRootDir = path |
Member
There was a problem hiding this comment.
One should never override dirs.GlobalRootDir manually, we need to use dirs.SetRootDir() to ensure all dependent paths are updated as well. But I don't think there's any point in having a dedicated MockFsRootPath at all, it's likely we can just call dirs.SetRootDir() directly in tests.
Contributor
Author
There was a problem hiding this comment.
That makes sense. I was wondering whether to use dirs.SetRootDIr() directly in the test or not and decided to do this to avoid changing of the other paths as well to mirror closer how it was mocked. I'll fix this.
Member
|
Also, it seems a |
github-actions
Bot
added
the
Needs Documentation -auto-
natibek
changed the title
Contributor
|
Looks good but tests seem to be failing, trying to re-run now with CI being more stable, if it still fails it might need a rebase. |
natibek
force-pushed
the
replace-rootPath-with-dirs.GlobalRootDir
branch
from
May 5, 2026 18:53
8ddf5e1 to
73be3a6
Compare
olivercalder
reviewed
May 5, 2026
olivercalder
left a comment
olivercalder
left a comment
Member
There was a problem hiding this comment.
Thanks for picking this up! A few comments
| tmpdir := c.MkDir() | ||
| restore := apparmor.MockFsRootPath(tmpdir) | ||
| defer restore() | ||
| dirs.SetRootDir(tmpdir) |
Member
There was a problem hiding this comment.
It's possible we'd want to do this in SetUpTest if we need to modify GlobalRootDir for other tests too, but if not, this is fine.
Contributor
Author
There was a problem hiding this comment.
This is the only test in version_test.go that modifies GlobalRootDir and it also needs to modify it for each iteration.
| // Pretend that apparmor kernel features directory doesn't exist. | ||
| restore := apparmor.MockFsRootPath(d) | ||
| defer restore() | ||
| dirs.SetRootDir(d) |
Member
There was a problem hiding this comment.
Yeah here it probably makes sense to do this in SetUpTest to avoid all the duplication across tests.
Contributor
Author
There was a problem hiding this comment.
Looking at the test file, there already is but it does not store the root path that has been set as the root directory for the test which some tests need. I'll make those changes.
| d := c.MkDir() | ||
| restore := apparmor.MockFsRootPath(d) | ||
| defer restore() | ||
| dirs.SetRootDir(d) |
Member
There was a problem hiding this comment.
Also, I think we need to do
defer dirs.SetRootDir("")(and elsewhere)
else tests will interfere with each other
Contributor
Author
There was a problem hiding this comment.
I'll add it to the test clean up.
| ) | ||
|
|
||
| func init() { | ||
| dirs.AddRootDirCallback(func(root string) { |
Member
There was a problem hiding this comment.
It's weird that dirs was already imported but rootPath was still a thing?
Contributor
Author
There was a problem hiding this comment.
Yeah, I noticed that as well. Strange
| var ( | ||
| // Filesystem root defined locally to avoid dependency on the 'dirs' | ||
| // package | ||
| rootPath = "/" |
Member
There was a problem hiding this comment.
it's troubling that there are no _test.go files which need changing in this package 🙃
Contributor
Author
There was a problem hiding this comment.
It is interesting that the tests in the cgroup package already use dirs correctly (they use SetUpTest to create a fake root and set it to the global root and add a cleanup function to restore it) .
Rnfudge02
reviewed
May 7, 2026
Rnfudge02
left a comment
Rnfudge02
left a comment
Contributor
There was a problem hiding this comment.
Looking good, a question
| } | ||
|
|
||
| func (s *apparmorSuite) TestSnapAppFromPidNewKernelPath(c *C) { | ||
| d := c.MkDir() |
Contributor
There was a problem hiding this comment.
Should there be a s.fakeroot assignment in this file? Similar to ln55-56 of apparmor_test.go
Contributor
Author
There was a problem hiding this comment.
The s.fakeroot is created in the SetUpTest function for apparmorSuite.
natibek
force-pushed
the
replace-rootPath-with-dirs.GlobalRootDir
branch
from
May 8, 2026 05:01
612bf66 to
4b6669d
Compare
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
4 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.
Replaces
rootPathvariable withdirs.GlobalRootDirin sandbox/apparmor. Pointed out in #15624 (comment).Tracked by: SNAPDENG-35226