tests/int: revamp (drop runc(), use run -N, add asserts) - #5429
Open
kolyshkin wants to merge 9 commits into
Open
tests/int: revamp (drop runc(), use run -N, add asserts)#5429kolyshkin wants to merge 9 commits into
runc(), use run -N, add asserts)#5429kolyshkin wants to merge 9 commits into
Conversation
kolyshkin
marked this pull request as draft
August 28, 2026 06:08
kolyshkin
changed the title
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
August 28, 2026 06:47
9357195 to
580166d
Compare
kolyshkin
changed the title
run -N, etc.)
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
August 28, 2026 07:53
e73b626 to
5fb7f31
Compare
kolyshkin
changed the title
run -N, etc.)run -N)
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
August 28, 2026 08:40
87cf0b4 to
5fb7f31
Compare
kolyshkin
marked this pull request as ready for review
August 28, 2026 08:40
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
August 29, 2026 02:37
d9deecf to
6bd8318
Compare
kolyshkin
changed the title
run -N)function runc, use run -N)
kolyshkin
changed the title
function runc, use run -N)runc(), use run -N, add asserts)
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
August 29, 2026 02:47
4d160e8 to
a5cbc11
Compare
kolyshkin
requested review from
lifubang and
rata
and removed request for
rata
September 1, 2026 00:07
kolyshkin
force-pushed
the
int-shim
branch
2 times, most recently
from
September 3, 2026 22:03
1bf73d7 to
60daed2
Compare
Contributor
Author
|
@AkihiroSuda @cyphar @thaJeztah @lifubang @rata PTAL (this changes our bats tests considerably, and so I have to rebase this manually every time we merge something involving bats tests). If you agree with the concept and the implementation, can we please merge this to reduce my load? One note on reimplementing asserts vs using bats' own -- unfortunately, they don't come with bats-core and the installation is a real PITA, on the other hand the re-implementation is pretty straightforward and probably wouldn't change much. Yet if you don't like it, I can remove the last few commits which add/use asserts. |
When runc is used as the right hand side of a pipe, it runs in a subshell, so the $status and $output set by bats' run helper (called from the runc wrapper) do not propagate to the test. As a result, the checks that follow such a call silently test the values left over from the previous runc call. Use a heredoc redirection instead of piping the heredoc in. Signed-off-by: Kir Kolyshkin <[email protected]>
We already have a function called runc, and adding a variable with the same name, while not technically incorrect, does not help readability. Fixes: a38f42a Signed-off-by: Kir Kolyshkin <[email protected]>
Until now, runc in integration tests was a bash function, wrapping the actual binary to add the flags all tests need (--root and, sometimes, --systemd-cgroup). A function can not be used where a real binary is required, which is why we also had setup_runc_cmdline and RUNC_CMDLINE, used by the tests that run runc under taskset or in a separate mount namespace. Add tests/integration/bin/runc, a small wrapper script doing the same thing, and prepend its directory to PATH. This way runc can be used both directly and after commands like taskset, nsenter, or timeout, and RUNC_CMDLINE is no longer needed. Note that the runc bash function still takes precedence for a plain "runc ..." call in tests, so their behavior is unchanged. Signed-off-by: Kir Kolyshkin <[email protected]>
With this option, bats prints the value of $output for a failed test, which is exactly what the sane_run wrapper does manually (and what the next commit removes it for). The option requires bats v1.7.0, so bump the minimum version. Our CI uses v1.12.0 on GHA and builds the same version from source on EL8; other platforms use their distro package, which is expected to be new enough (and bats_require_minimum_version makes it obvious if it isn't). Signed-off-by: Kir Kolyshkin <[email protected]>
Now that runc is a real binary (a wrapper script in PATH), tests can
call it the way any other command is called, using bats' run helper
directly:
run -0 runc start ctr
instead of the old
runc start ctr
[ "$status" -eq 0 ]
where runc was a bash function hiding a call to run. The exit code check
is now part of the call: -N for an expected exit code, ! for "any
failure", and no argument where the exit code is checked later or not at
all.
This has to be done in one go: while the runc function exists, "run runc"
would call it, resulting in a nested run.
Other changes made necessary or possible by this:
- The runc and sane_run functions are gone. The output of a failed
command is now printed by bats itself, see the previous commit.
- __runc (a raw runc call, not using run) is now simply runc.
Signed-off-by: Kir Kolyshkin <[email protected]>
A fair number of runc calls do not check the exit code at all, although they should. Fix that. Note that two of the checks added to update.bats expect an error (exit code 1), as the test cases there are about runc rejecting a combination of options. The remaining calls that do not check the exit code are the ones that check $status themselves (in an if, or against different values), the one in a background subshell (where a failure can not be detected), and the one that is explicitly documented as being allowed to fail. Signed-off-by: Kir Kolyshkin <[email protected]>
When a test fails on a string comparison, all we see is the failed expression, which usually means adding some debug output and rerunning the test to find out what the actual value was. The bats-assert library solves this, but it (together with its bats-support dependency) has to be installed separately on every distro we test on, which is more trouble than it's worth for a few functions. Add assert_output, refute_output, and assert_line, implementing a subset of the bats-assert API (so that the tests read the same way, and this code can be dropped should bats-assert ever be added as a dependency), and use them for the comparisons against an exact value. Now, when such a check fails, the test log shows what was expected and what was there: -- output does not match (exact) -- expected: '/override' actual: $'\0/override' -- Signed-off-by: Kir Kolyshkin <[email protected]>
The point of the assert helpers is that a failed test says what it wanted and what it got, so leaving most comparisons as bare [[ ]] defeats it. Convert the rest of the checks on $output and $lines. Those using a glob to look for a substring become --partial, the rest become --regexp (the patterns are translated to extended regular expressions, and anchored where the glob was). The few remaining [[ ]] are not assertions: one is an if condition, and another one is the left hand side of ||, offering an alternative to the assert that follows it. Signed-off-by: Kir Kolyshkin <[email protected]>
The "Writing integration tests" section only pointed at helpers.bash, which does not say much about how a test is supposed to be written. Now that tests call runc via bats' run helper and check the output with assert_output and friends, document that: how the expected exit code is given to run, which assertions are available, and why they are preferred over a bare test expression. Signed-off-by: Kir Kolyshkin <[email protected]>
Member
|
Sorry, looks like I missed the previous ping; are the failures expected, or something broken in the current PR? |
thaJeztah
reviewed
Sep 8, 2026
Comment on lines
+118
to
+114
| sane_run taskset -c "$first" "${RUNC_CMDLINE[@]}" run ctr | ||
| sane_run taskset -c "$first" runc run ctr |
Member
There was a problem hiding this comment.
I like that it looks and feels as a direct invocation of runc, but this may also be a potential downside; would a case where .. for some reason .. runc is invoked directly be immediately apparent?
Member
There was a problem hiding this comment.
(in that case, we could rename the script to something more clear to be the wrapper script)
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
2 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.
This supersedes #4946, taking a different approach to the problems
discussed there (see this comment and below).
The end result is that integration tests call runc the way any other
command is called, and check its output with helpers that say what went
wrong:
run -0 runc start ctr assert_output --partial "something"instead of the current
where
runcis a bash function hiding a call to bats'runhelper, anda failed comparison only shows the expression, not the values.
What a failure looks like now
To see this in practice, four deliberate bugs were introduced (
runc -vprinting
spec :,runc list -qadding a trailing space to the ID, areworded hook error, and
runc delete --forcefailing on a missingcontainer), and the tests were run with and without this PR:
list -qadds a trailing spaceexpected: 'test_box1'actual: 'test_box1 'spec :instead ofspec:runc -voutput to eyeballexpected: 'spec: [0-9]+\.[0-9]+\.[0-9]+'actual: 'spec : 1.3.0'expected: 'error running createRuntime hook #1:'actual: '...failure in createRuntime hook #1...'delete --forcefails[ "$status" -eq 0 ]' failed-- but after which call?run -0 runc delete --force notexists' failed, expected exit code 0, got 1The trailing space one is the point of the whole exercise. Before:
The expected value is not shown, the actual one looks correct, and the
difference is invisible. Now: