test(hypervisors): cover Qemu.BuildExecCmd#597
Conversation
Walks through the main code paths the function exercises — static flags, memory, vCPUs, network (incl. vhost), initrd, sharedfs, block devices, vsock, and the kernel command append rule. Signed-off-by: Parth Dagia <parth.24bcs10414@sst.scaler.com>
✅ Deploy Preview for urunc canceled.
|
There was a problem hiding this comment.
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.BuildExecCmdmultiple 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.
Per review feedback: collapses the per-feature test functions into a single TestQemuBuildExecCmd with table-driven subtests, so each case calls BuildExecCmd exactly once and runs all of its assertions on the result. Signed-off-by: Parth Dagia <parth.24bcs10414@sst.scaler.com>
|
@cmainas the per-feature test functions are now collapsed into a single TestQemuBuildExecCmd, the coverage is same as before |
cmainas
left a comment
cmainas
left a comment
There was a problem hiding this comment.
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.
|
|
||
| // 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 was a problem hiding this comment.
There is no need to pass a function to change the value of a struct.
| mustContain: []string{"-smp 1"}, | ||
| }, | ||
| { | ||
| name: "VCPUs=4 emits -smp 4", |
There was a problem hiding this comment.
This is the same test as the above.
| mustContain: []string{"-smp 4"}, | ||
| }, | ||
| { | ||
| name: "VCPUs=16 emits -smp 16", |
There was a problem hiding this comment.
This is the same test as the above.
|
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. |
2 participants
Description
Qemu.BuildExecCmdinpkg/unikontainers/hypervisors/qemu.godidn't have any direct unit-test coverage, so this PR addsqemu_test.gothat walks through the main branches the function actually exercises:-smpwhen zero, sets it correctly otherwise)-M virton arm64)MonitorNetCli, and vhost on/offExactArgsverbatim pathMonitorCliextras (ExtraInitrd,OtherArgs)VAccelTypeset-appendrule for the kernel command lineA small
fakeUnikernelstub keeps the tests focused onBuildExecCmditself rather than pulling in any real unikernel implementation.Related issues
How was this tested?
go test ./pkg/unikontainers/hypervisors/...passes locallygo build ./...passest.Parallel()and assert against the joined command stringLLM 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
make lint).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).