mountinfo: TestMountedBy: skip mountedByOpenat2() check if not supported #67
Conversation
|
@kolyshkin PTAL; noticed this when running tests in a container on Docker Desktop (kernel 4.19) |
|
ehm. probably don't need this as #65 also fixes this 🤦♂️thought we already merged that one |
|
Merged #65 |
|
As I said in #65, I definitely had openat2 way in mind when writing this, mostly added mountinfo to test the test itself. As we already test this functionality on an openat2-capable system that should be sufficient, i.e. this check does not bring much by itself. Feel free to either enable part of GetMountedBy test, or close this PR. |
thaJeztah
force-pushed
the
skip_no_openat2
branch
from
f1bd5f0 to
8904c79
Compare
|
I updated the PR, but would be fine either way (closing / merging). ptal |
mountinfo/mounted_linux_test.go
Outdated
| func TestMountedBy(t *testing.T) { | ||
| requireOpenat2(t) | ||
| func tryOpenat2(t *testing.T) error { | ||
| t.Helper() |
There was a problem hiding this comment.
AFAIK it does not have to be a helper (so also no need to pass t).
There was a problem hiding this comment.
Yeah, probably not strictly needed; I usually add it by default, so that issues get reported with the line of the Test, not the line in the helper. I'll remove
mountinfo/mounted_linux_test.go
Outdated
| } else if mounted != exp { | ||
| t.Errorf("mountedByOpenat2(%q): expected %v, got %v", m, exp, mounted) | ||
|
|
||
| err = tryOpenat2(t) |
There was a problem hiding this comment.
It doesn't matter much really, but calling it in a loop seems unclean -- e.g. it could make a reviewer think that something we do here might affect the value returned from tryOpenat2.
So I would move it out of the loop, doing something like
openat2Supported = tryOpenat2(t) == niland below
if openat2Supported {
mounted, err = mountedByOpenat2(m)
...
Left some notes, also the commit description needs an update. |
thaJeztah
force-pushed
the
skip_no_openat2
branch
from
8904c79 to
cbec9c5
Compare
|
@kolyshkin updated; ptal |
|
LGTM except the commit message seems to be not up-to-date (it was before #65). |
@thaJeztah PTAL ^^^ 🙏🏻 |
This updates the test to only skip the mountedByOpenat2() part, instead of skipping the whole test, if openat2 is not supported. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
skip_no_openat2
branch
from
cbec9c5 to
098ed00
Compare
|
@kolyshkin updated, PTAL |
thaJeztah
changed the title
3 participants
This updates the test to only skip the mountedByOpenat2() part, instead
of skipping the whole test, if openat2 is not supported.