tests: xfail integration tests that fail without buildbox-fuse #1632
Conversation
abderrahim
requested review from
gtristan,
juergbi,
BenjaminSchubert and
cs-shadow
as code owners
abderrahim
force-pushed
the
abderrahim/fuse-xfail
branch
from
a28a056 to
5a202ce
Compare
| @@ -171,6 +172,7 @@ def test_script_layout(cli, datafiles): | |||
| HAVE_SANDBOX == "buildbox-run" and BUILDBOX_RUN == "buildbox-run-userchroot", | |||
| reason="Root directory not writable with userchroot", | |||
| ) | |||
| @pytest.mark.xfail(not HAVE_BUILDBOX_FUSE, reason="Root directory not writable without buildbox-fuse") | |||
There was a problem hiding this comment.
Does it really make sense to xfail rather scenarios that cannot possibly work? Doesn't it just make everything slower?
There was a problem hiding this comment.
Normally we use @pytest.mark.skipif to conditionally skip tests which we cannot run because of lacking components in the environment.
However here it seems appropriate to conditionally xfail the test since this should be considered incorrect behavior of buildstream regardless of the absence of buildbox-fuse, so it's nice to have the tests complain about this.
There was a problem hiding this comment.
Hmm, but one the other hand, this incorrect behaviour seems expected and this test is just documenting it.
There was a problem hiding this comment.
If I remember correctly, the non-writable root directory is only an expected failure with userchroot (which we already mark as xfail). If this is not working with bubblewrap without buildbox-fuse, we should at least investigate this before marking it as xfail.
There was a problem hiding this comment.
Thinking about this, I think it's an expected failure without buildbox-fuse. I don't know much about userchroot, but I don't think it's related: if we have a the fuse layer to protect the underlying blobs from corruption we can allow their modification. If not we'll simply protect them by not allowing writing (of course, this will only protect from accidental modification and you need to run buildbox-casd as a different user to protect against malicious modification).
I think the relationship between buildbox-fuse and buildbox-run-userchroot is a correlation (if you're using userchroot you're likely not on linux so you can't use fuse) rather than a strict relation.
Did I miss something?
There was a problem hiding this comment.
Looking at the test case in more detail, this is not actually related to the writability of the root directory. It's about the possibility to modify an existing staged file (without removing it beforehand and without causing cache corruption). The userchroot xfail condition (and reason) makes sense for test_script_root, however, for the corruption tests, a different approach makes more sense:
Drop the xfail (even the existing userchroot one). Instead, expect the build of element_name to fail if buildbox-fuse is not used. That build should fail with a proper setup (different user for buildbox-casd). This is more precise than an xfail test marker as that way we can make sure that it fails at the right point (we don't want it to xfail if it can corrupt the canary). I haven't tested this, though.
Two of them are known (they are already marked as xfail when using buildbox-run-userchroot) but the third seems weird. It's probably a bug.