Skip to content

tests: xfail integration tests that fail without buildbox-fuse #1632

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tests: xfail integration tests that fail without buildbox-fuse #1632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/buildstream/_testing/_utils/site.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@
except (ProgramNotFoundError, OSError, subprocess.CalledProcessError):
pass

try:
utils.get_host_tool("buildbox-fuse")
HAVE_BUILDBOX_FUSE = True
except ProgramNotFoundError:
HAVE_BUILDBOX_FUSE = False


# Check if we have subsecond mtime support on the
# filesystem where @directory is located.
Expand Down
5 changes: 4 additions & 1 deletion tests/integration/script.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from buildstream import _yaml
from buildstream._testing import cli_integration as cli # pylint: disable=unused-import
from buildstream._testing._utils.site import HAVE_SANDBOX, BUILDBOX_RUN
from buildstream._testing._utils.site import HAVE_SANDBOX, BUILDBOX_RUN, HAVE_BUILDBOX_FUSE


pytestmark = pytest.mark.integration
Expand Down Expand Up @@ -95,6 +95,7 @@ def test_script_root(cli, datafiles):

@pytest.mark.datafiles(DATA_DIR)
@pytest.mark.skipif(not HAVE_SANDBOX, reason="Only available with a functioning sandbox")
@pytest.mark.xfail(not HAVE_BUILDBOX_FUSE, reason="I don't know, seems like a bug")
def test_script_no_root(cli, datafiles):
project = str(datafiles)
element_path = os.path.join(project, "elements")
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really make sense to xfail rather scenarios that cannot possibly work? Doesn't it just make everything slower?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but one the other hand, this incorrect behaviour seems expected and this test is just documenting it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@juergbi juergbi Apr 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def test_regression_cache_corruption(cli, datafiles):
project = str(datafiles)
checkout_original = os.path.join(cli.directory, "checkout-original")
Expand Down Expand Up @@ -215,6 +217,7 @@ def test_regression_tmpdir(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")
def test_regression_cache_corruption_2(cli, datafiles):
project = str(datafiles)
checkout_original = os.path.join(cli.directory, "checkout-original")
Expand Down