test(hypervisors): cover Qemu.BuildExecCmd by parthdagia05 · Pull Request #597 · urunc-dev/urunc

parthdagia05 · 2026-05-01T18:57:59Z

Description

Qemu.BuildExecCmd in pkg/unikontainers/hypervisors/qemu.go didn't have any direct unit-test coverage, so this PR adds qemu_test.go that walks through the main branches the function actually exercises:

Static flags that should always be present
Memory default fallback and custom values
vCPU handling (omits -smp when zero, sets it correctly otherwise)
Seccomp on / off
Architecture-specific machine type (-M virt on arm64)
Networking: empty TapDev, generic tap setup, custom MonitorNetCli, and vhost on/off
Initrd present / absent
Sharedfs: 9pfs, virtiofs, and none
Block devices: no blocks, generic, and ExactArgs verbatim path
MonitorCli extras (ExtraInitrd, OtherArgs)
vsock with VAccelType set
The -append rule for the kernel command line

A small fakeUnikernel stub keeps the tests focused on BuildExecCmd itself rather than pulling in any real unikernel implementation.

Related issues

Fixes #

How was this tested?

go test ./pkg/unikontainers/hypervisors/... passes locally
go build ./... passes
All sub-tests use t.Parallel() and assert against the joined command string

LLM usage

Used Anthropic's Claude Opus 4.6 to help research the function's branches and shape the test cases around the actual code paths.

Checklist

I have read the contribution guide.
The linter passes locally (make lint).
The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
If LLMs were used: I have read the llm policy.

netlify · 2026-05-01T18:58:05Z

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	`4aee235`
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a1ec166c226730008d0e913

cmainas

Hello @parthdagia05 ,

thank you for this PR. Please do not use multiple test functions for the same test and:

The tests could be a single test with subtests using table driven tests (See https://github.com/parthdagia05/urunc/blob/test/hypervisors-qemu-buildexeccmd/pkg/unikontainers/vaccel_test.go#L23 for reference).
There is no reason to call q.BuildExecCmd multiple times with the same parameters.
In one tests you can have multiple checks, See https://github.com/parthdagia05/urunc/blob/test/hypervisors-qemu-buildexeccmd/pkg/unikontainers/block_test.go for example.

parthdagia05 · 2026-05-05T09:46:35Z

@cmainas the per-feature test functions are now collapsed into a single TestQemuBuildExecCmd, the coverage is same as before

cmainas

Hello @parthdagia05 ,

the new version is very complex without any reason. We want to test one function, which takes a struct as an argument. The struct which we pass as an argument controls the result. However, not all the fields of the struct affect the result. Therefore, we can focus only in the fields that affect the result.

Also, there are some specific values, like the qemu binary name or the argument of -kernel. These can be tested in a single test.

cmainas · 2026-05-08T14:47:52Z

+
+// withArgs returns defaultArgs() after applying the given modifier, so that
+// each table case can declare only the fields it cares about.
+func withArgs(modify func(*types.ExecArgs)) types.ExecArgs {


There is no need to pass a function to change the value of a struct.

cmainas · 2026-05-08T14:48:24Z

+			mustContain: []string{"-smp 1"},
+		},
+		{
+			name:        "VCPUs=4 emits -smp 4",


This is the same test as the above.

cmainas · 2026-05-08T14:48:30Z

+			mustContain: []string{"-smp 4"},
+		},
+		{
+			name:        "VCPUs=16 emits -smp 16",


This is the same test as the above.

parthdagia05 · 2026-05-08T15:00:33Z

Hi @cmainas, thanks for your feedback, I think I jumped into writing tests before fully understanding the patterns. Let me take a proper look through rest of the existing tests and update accordingly. I'd rather slow down and get the structure right than ship another iteration that misses something obvious.

parthdagia05 · 2026-05-14T17:35:49Z

@cmainas done with the changes.

cmainas

Thank you @parthdagia05 for the changes.

I believe we can remove the "extra" check defined in two tests and perform the same tests either directly in the test loop or with mustContain.

cmainas · 2026-06-02T08:28:56Z

+				} else {
+					assert.NotContains(t, strings.Join(out, " "), archFlag)
+				}
+			},


No reason for this to be an "extra" check. We can check it in the subtest loop instead.

cmainas · 2026-06-02T08:31:54Z

+			extra: func(t *testing.T, out []string) {
+				assert.GreaterOrEqual(t, len(out), 2)
+				assert.Equal(t, "-append", out[len(out)-2])
+				assert.Equal(t, "init=/bin/sh root=/dev/vda console=ttyS0", out[len(out)-1])


We do not need an "extra test for this". If we include the whole command in mustContain, then we can be sure that it is a single argument.

parthdagia05 · 2026-06-02T08:55:19Z

@cmainas removed the extra check

cmainas

Hello @parthdagia05 ,

I have one more question. Sorry for not checking this earlier.

cmainas · 2026-06-02T10:46:57Z

+
+			// Invariants that hold for every call to BuildExecCmd.
+			assert.Equal(t, testQemuBinary, out[0], "binary path must be the first element")
+			joined := strings.Join(out, " ")


Is there a reason for joining the string, instead of check if out contains the respective strings?

Because BuildExecCmd does strings.Split(cmdString, " ") (qemu.go-140th line), multi-word flags like -m 256M end up as two separate slice elements, so assert.Contains(out, "-m 256M") wouldn't match so i thought joining back is the simplest way to substring-check them as one piece.

Yeah, this makes sense. We need to come up with a better scheme for the construction of the cli args.

cmainas · 2026-06-02T11:37:06Z

Thank you @parthdagia05 for the changes. Can you rebase over main and squash your commits so we can merge?

Adds a table-driven test that covers the main branches of BuildExecCmd: static flags, memory default and custom value, vCPUs, seccomp, the architecture machine flag, networking (no-net, generic tap, custom MonitorNetCli, vhost), initrd, sharedfs (9pfs / virtiofs), block devices (generic and ExactArgs), MonitorCli extras, vsock, and the kernel command append rule. A small fakeUnikernel stub keeps the tests focused on BuildExecCmd itself. Signed-off-by: Parth Dagia <parth.24bcs10414@sst.scaler.com>

parthdagia05 · 2026-06-02T11:43:22Z

Done @cmainas

cmainas

Thank you @parthdagia05 for the tests.

Adds a table-driven test that covers the main branches of BuildExecCmd: static flags, memory default and custom value, vCPUs, seccomp, the architecture machine flag, networking (no-net, generic tap, custom MonitorNetCli, vhost), initrd, sharedfs (9pfs / virtiofs), block devices (generic and ExactArgs), MonitorCli extras, vsock, and the kernel command append rule. A small fakeUnikernel stub keeps the tests focused on BuildExecCmd itself. PR: #597 Signed-off-by: Parth Dagia <parth.24bcs10414@sst.scaler.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>

cmainas requested changes May 5, 2026

View reviewed changes

cmainas reviewed May 8, 2026

View reviewed changes

cmainas reviewed Jun 2, 2026

View reviewed changes

parthdagia05 force-pushed the test/hypervisors-qemu-buildexeccmd branch from 8396e65 to 4aee235 Compare June 2, 2026 11:41

cmainas added ok-to-test takeover labels Jun 9, 2026

urunc-bot Bot changed the base branch from main to main-pr597 June 9, 2026 12:42

cmainas approved these changes Jun 9, 2026

View reviewed changes

cmainas merged commit 3a88a70 into urunc-dev:main-pr597 Jun 9, 2026
53 of 55 checks passed

Conversation

parthdagia05 commented May 1, 2026

Description

Related issues

How was this tested?

LLM usage

Checklist

Uh oh!

netlify Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for urunc canceled.

Uh oh!

cmainas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthdagia05 commented May 5, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthdagia05 commented May 8, 2026

Uh oh!

parthdagia05 commented May 14, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthdagia05 commented Jun 2, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmainas commented Jun 2, 2026

Uh oh!

parthdagia05 commented Jun 2, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

netlify Bot commented May 1, 2026 •

edited

Loading

cmainas left a comment •

edited

Loading