Skip unit test when prjquota not supported#51083
Conversation
daemon/internal/quota/testhelpers.go
Outdated
| // CanTestQuota - checks if xfs prjquota can be tested | ||
| // returns a reason if not | ||
| func CanTestQuota() (string, bool) { | ||
| func CanTestQuota(t *testing.T) (string, bool) { |
There was a problem hiding this comment.
Doesn't seem necessary to pass t *testing.T just for t.TempDir()
| func CanTestQuota(t *testing.T) (string, bool) { | |
| func CanTestQuota() (string, bool) { |
Alternatively you can just omit return values and call t.Skip() from this function
| func CanTestQuota(t *testing.T) (string, bool) { | |
| func RequireSupported(t *testing.T) { |
There was a problem hiding this comment.
Just a quick blurb / thinking out loud; I haven't checked exactly how we set things up for tests, but I think the location used for daemons to run tests is configurable; in that case, we may need to pass the location to test, as it could be on a different filesystem.
(I recall we had some checks in graph-drivers that created a tempDir to test if they were supported, but didn't account for (e.g. /var/lib/docker/XXX to be a different storage / mount attached to the machine)
There was a problem hiding this comment.
Sorry for the delay – I've been under the weather for a few days.
I'm not sure I understand your comment fully.
I found this issue (that you submitted to GO) linked in a comment (daemon/daemon_test.go:345). Seems like there's some trickiness with temp directories in MacOS. Is this related to what you're talking about?
The code at this particular comment resolves symlinks using filepath.EvalSymlinks(t.TempDir()). However, this seems to be the only case where this is done. As far as I can tell, there seems to be no special handling of t.TempDir() anywhere else.
Would you mind expanding on your point?
My thinking was that it simplifies clean-up. Calling t.Skip() from inside the helper would be hiding control flow in my eyes, but I think with a re-name to RequireSupported (or something similar) it's fine enough.
I'll go ahead and change it once I hear back from @thaJeztah
There was a problem hiding this comment.
Went ahead and updated the PR. Function now skips the test directly.
There was a problem hiding this comment.
Just a quick blurb / thinking out loud; I haven't checked exactly how we set things up for tests, but I think the location used for daemons to run tests is configurable; in that case, we may need to pass the location to test, as it could be on a different filesystem.
@thaJeztah - maybe you're thinking of $DOCKER_INTEGRATION_DAEMON_DEST? If so, it's probably ok .. the temp dir here is only used as a mount point, to check the kernel can mount a filesystem with the prjquota option. The filesystem it's mounted in to isn't important?
There was a problem hiding this comment.
Hope you don't mind be pinging you @thaJeztah, but would you mind having a look at Rob's comment above?
I'd be happy to look in to your question, but I would need some more info to go on.
fd4118c to
0fba10d
Compare
0e43768 to
2a019c1
Compare
Signed-off-by: Oskar Arensmeier <oskar.arensmeier@outlook.com>
2a019c1 to
66f7a48
Compare
robmry
left a comment
There was a problem hiding this comment.
LGTM - but will wait to see if @thaJeztah's question is answered before merging.
Closes #51065.
- What I did
Added an additional prerequisite check before running
daemon/internal/quotaunits tests. This fixes an issue where the tests could run in environments where they could never succeed.In particular, I extended the
CanTestQuotato also attempt a mount using theprjquotaoption. This ensures that XFS actually supports the quota option before running tests.- How I did it
Added a function called
supportsQuotathat attempts a mount. It reuses some existing logic to prepare the XFS image (PrepareQuotaTestImage).- How to verify it
Run the quota unit tests
TESTDIRS='./daemon/internal/quota/' ./hack/test/unitThe outcome depends on what the backing filesystem supports
Unfortunately I don't have easy access to a system where XFS support quotas, so I've been unable to verify the last scenario.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
Baby moose