Repository navigation
nvx-rs: run the guest-binary tests' nvx-time hooks with one helper - #637
Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Conversation
tests/guest_tools.rs runs nvx-time's test hooks on fixture files at 18 sites, and each spelled out the hook's command line, `nvx-time test HOOK FIXTURE ARGUMENTS...`, as an array of OsStr values: "test", the hook, the fixture's path, and the hook's arguments. One of the sites was already a helper, run_time_packet, which writes a packet fixture and runs the packet hook on it, yet the three packet sites of time_test_mode_decodes_packets_samples_and_arithmetic built the same command line by hand. time_hook(hook, fixture, arguments), next to time_fixture, which writes the fixtures, now builds that command line once and runs it with run. run_time_packet keeps its signature and calls it, and so do the other 17 sites: the packet, sample, boot-log, kmsg, timer-list, state, status, cpuinfo, and restore-record hooks, and the run_log closure, which runs the boot-log and syslog hooks. Each call names only what varies, and the file loses 106 lines. The nvx-time event hook, which takes no fixture, and the nvx-ingress test hook keep their own command lines. Behavior is unchanged: through time_hook, each site hands run the arguments that it passed before, in the same order, so every test runs the same command and checks the same exit status and output. Only the test changes; the library, the CLI, and the guest binaries build from unchanged sources. The guest_tools tests cover the change. On Linux, the all-features test run, check-guest-binaries, and test-guest-binaries against a guest-profile build of the binaries each ran all 34 of them, and all passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The test-only refactor preserves existing behavior, with no unresolved issues identified.
No open issues 🎉
What changed in this PR
Consolidates fixture-based nvx-time test commands into one helper without changing production code or test expectations.
Changes:
- Adds
time_hookand routes 18 command-construction sites through it. - Preserves argument order, native fixture paths, and existing assertions.
| File | Description |
|---|---|
nvx-rs/tests/guest_tools.rs |
Shares hook command construction across guest-binary tests. |
Review effort: Balanced
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
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.
Summary
nvx-rs/tests/guest_tools.rsrunsnvx-time's test hooks on fixture files at 18 sites, and each spelled out the hook's command line,nvx-time test HOOK FIXTURE ARGUMENTS..., as an array ofOsStrvalues:OsStr::new("test"),OsStr::new(HOOK),fixture.as_os_str(), and anOsStr::newfor each argument, in 4 to 10 lines. One of the sites was already a helper,run_time_packet, which writes a packet fixture and runs thepackethook on it, yet the threepacketsites oftime_test_mode_decodes_packets_samples_and_arithmeticbuilt the same command line by hand.time_hook(hook, fixture, arguments), next totime_fixture, which writes the fixtures, now builds that command line once and runs it withrun.run_time_packetkeeps its signature and calls it, and so does every other site, so each call names only the hook, the fixture, and the arguments that vary. The hook's command line now has one definition, and the file loses 106 lines.This is the nineteenth
nvx-rs-quality/pull request. #603, #605, #606, #607, #608, #609, #611, #613, #614, and #615 merged, and #616, #617, #618, #632, #633, #634, #635, and #636 are open. Each had a Copilot overview review and no comment to follow, and none was closed without merging. Those runs changed the EXEC and MAPS codecs, the managed agent, the guest tools andguest/sys,host/artifacts.rs,host/filesystem.rs,host/session.rs, the CLI,host/sandbox, the fake OpenVMM, and the NVXS integration test. This run looked at the parts that they left alone:host/network,host/python,host/instance.rs,host/launch.rs,host/nvxs.rs,net,protocol/nvxs,guest/time,guest/time_probe,guest/ingress,guest/nvxs.rs, andtests/guest_tools.rsandtests/cli.rs. It surveyed them by reading, with Clippy's pedantic lints as hints, and with a throwaway script, kept outside the repository, that reports every repeated window of six or more normalized lines insrcandtests, exactly and with names and literals abstracted, and once more over production code alone. It skipped every file that an open pull request todevchanged when it started: those of the opennvx-rs-quality/pull requests above; those of #625, #628, #629, and #631 (Cargo.toml,README.md,src/lib.rs, the managed agent,guest/mod.rs,guest/sys/{fd,mod,spawn}.rs,guest/time_probe/clock.rs,host/mod.rs,host/platform,host/process.rs,host/sys, the newhost/storeandguest/spawn.rs, andtests/store.rsandtests/supervisor.rs); anddoc/project-structure.md, which #626, #628, #629, and #630 change.Of the candidates, this one removes the most code per changed line, 106 lines in 182, by generalizing a helper that the file already had, in a file that no earlier run touched. The production-code candidates remove less: a shared
mainfor the four guest programs whoseruntakes the arguments and both standard streams would remove about 15 lines in about 45, andparse_legacy_rule's reuse oflowering::port_selectorsabout 8 in 20. Follow-ups lists them and the others.Where the choice was open, I took the conservative option:
run_time_packetstays, with its signature, because its six callers intime_test_mode_checks_packet_and_sample_boundariespass it a directory, a name, and packet bytes; only its body changes.packetsites oftime_test_mode_decodes_packets_samples_and_arithmetictherefore calltime_hookon the fixtures that they already wrote, rather thanrun_time_packet.nvx-timeeventhook, which takes no fixture and passes an argument that is not UTF-8, and thenvx-ingresstesthook, of another program, keep their command lines. A helper for them would be a change of its own.time_hooktakes its arguments as&str, as every site passed them, and is#[cfg(target_arch = "x86_64")], as the file's othernvx-timehelpers are.Changes
All changes are in
nvx-rs/tests/guest_tools.rs: 38 insertions and 144 deletions.time_hook, aftertime_fixture, with a doc comment: chainstest, the hook, and the fixture's path with the arguments, and runsnvx-timewithrun.time_test_mode_decodes_packets_samples_and_arithmetic: thepackethook on thepacket,ranges, andno-rangesfixtures, and thesamplehook;time_test_mode_checks_logs_timer_state_and_status: theboot-log,kmsg, andtimer-listhooks, thestatehook on the valid and the malformed state, and thestatushook;time_test_mode_preserves_boot_log_and_syslog_results: therun_logclosure, which runs theboot-logandsysloghooks;run_time_packet, which the sixpacketcases oftime_test_mode_checks_packet_and_sample_boundariescall, and that test's twosamplesites;time_test_mode_covers_checks_events_records_and_workers: the twocpuinfosites;time_test_mode_covers_remaining_events_records_and_workers: the tworestore-recordsites.No library item, binary, message, error kind, protocol byte, fixture, or file outside
tests/guest_tools.rschanges.Behavior
time_hookpassesrunOsStr::new("test"),OsStr::new(hook),fixture.as_os_str(), andOsStr::newof each argument, in that order: the values that each site passed, in its order.runspawnsnvx-timewith them as before, so every test runs the same command and compares the same exit status, standard output, and standard error with the same expectations.runand every assertion are unchanged, and no test is added, removed, skipped, or weakened:guest_toolsstill runs 34 tests.nvx-rsCLI, and the guest binaries build from unchanged sources: the Alpine initramfs built here from the guest binaries of TREE has the SHA-256 that nvx-rs: encode the NVXS tests' records with one function #636 reported for its own build,4ef90a58ebbc2abfaf47474147972eb2d0cfe57d087c4d7af58bcecff4207a00.Tests that cover the change: the 34
guest_toolstests, which calltime_hookat every site above. On Linux,cargo test --all-features,check-guest-binaries, andtest-guest-binariesagainst a guest-profile build of the binaries each ran all 34, and all passed.Follow-ups
The survey turned these up, and this change leaves them alone. I found no suspected bug.
tests/guest_tools.rs. The fivenvx-ingress test DIRECTORYsites spell out their command line as thenvx-timesites did, anddevice_iorepeatsrun's spawn and collection to keep theExitStatusfor its message.nvx-console-pending,nvx-ingress,nvx-time, andnvx-time-probehave the same seven-linemain, which restoresSIGPIPEand turns the result ofrun(&sys::args(), &mut sys::Stdout::new(), &mut io::stderr())into an exit code.guest/status.rscould hold it.nvx-device-ioandnvx-managed-agent, whose files open pull requests change, start differently, and the tests ofnvx-timeandnvx-time-proberepeat oneargshelper.protocol/nvxs/tests/guest.rsandhost.rswriteIdentity { epoch: N, ..IDENTITY }13 times, andtests.rsonce more, and both list the same two identities that lack an instance or an epoch.host/network/policy/mod.rs.parse_legacy_rulestill writes out theportbranch oflowering::port_selectors. Reusing it would letport_protocol_selectorsfold intoport_selectors.host/python/fs.rs. The Windowsstatandlstatbuild the sameStat, and the POSIXstatusreturns aResultthat never fails.tests/nvxs/broker.rs,protocol/nvxs/tests.rs,netandhost/network/mod.rs,host/instance.rs,guest/nvxs.rs,host/python/dumps.rs,guest/time/test_mode.rs, the sandbox test helpers,host/launch.rs, the time tools,guest/ingress, andtests/cli.rs.Validation
BASE:
6cb1189dec42b247062790623d0ca85b716316f1, which wasorigin/devwhen the run started: the merge of build: reject Ubuntu initramfs source edits made during packing #584. Before this pull request was opened,origin/devhad advanced toaface705dby the merges of guest: match C hexadecimal output for zero clock values #631 and nvx-rs: add verified process references with event-driven exit waits #625 and a performance-baseline update, none of which changestests/guest_tools.rs, and no open pull request todevchanges it.TREE:
51709d6584a619148322918ac51f13a55a59f312, the tree of this commit. Every host applied the patch to BASE and checked thatgit write-treeequals TREE.Release on every host:
v0.1.0-dev.09d60f9c8ee6, from the merge of nvx-rs: check the guest agent's replies through shared methods #615, the nearest first-parent ancestor of BASE that had packages, two commits earlier. Its OpenVMM,d0bdbed8f6e3b86271277dc22658109e2e633b2d, is the pinned revision.downloadinstalled the release'snvx-rs, whose sources equal BASE's. Each host built theopenvmm_e2eandmanaged_lifecycletest binaries after its other Cargo work, and its hypervisor test logs show no compilation.Guest artifacts were rebuilt from TREE on prometheus30, whose user namespaces let Alpine's apk run its scripts, with these steps:
build-guest-binaries --output;test-guest-binaries --binaries, where 34 tests passed;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-2564ef90a58ebbc2abfaf47474147972eb2d0cfe57d087c4d7af58bcecff4207a00), its packages manifest, and its provenance went through this machine to the other two hosts, where the SHA-256 digests matched.Local checks on Windows with Rust 1.95.0:
-D warnings, with all targets and features, for Windows and forx86_64-unknown-linux-gnu, which compilestests/guest_tools.rs, and on theguest_toolstest forx86_64-unknown-linux-gnuwith onlyguest;aarch64-apple-darwin) checks, for bothnvx-rsandaci_edge_sandboxes.The change adds, moves, and deletes no file and changes no Markdown, so the CLI tests of validate-nvx and the link check had nothing to cover.
test-aci-edge-sandboxestest-microvm, default suitecheck-guest-binaries(361 tests, 34 of themguest_tools), 657nvx-rstests with all features (34 of themguest_tools) and 308 with the default ones, artifact bundling (15), 158 and 85aci_edge_sandboxestests, and agent bundling. The macOS checks ran locallynvx-rstests with all features and 308 with the default ones, artifact bundling (15), 158 and 85aci_edge_sandboxestests, and agent bundling. The MSRV checks ran locallyEvery gate and test passed at its first attempt. The Linux gates ran in the foreground of their SSH sessions with
SIGINTat its default disposition, so the CLI's Ctrl-C tests ran as intended; the sessions' shells ignored onlySIGQUIT(SigIgn: 0x4). Each suite'smanaged-lifecyclescenario ran themanaged_lifecycletest with Cargo, which passed. OpenVMM warned of a host CPU-profile fallback only in thecpu-profile-fallbackscenario, which forces one. The time-ABI evidence lines counted no capture or restore over its CPU-time budget, and no boot over it on prometheus28. On prometheus32 and prometheus30, one boot of 58 each went over it (boot_cpu_usup to 17,939 and 10,015 µs). The suite reports that count without gating on it, and this change leaves the time tools and the guest binaries, which built to the bytes of #636's, alone.Left to CI:
test-openvmm-unitandtest-openvmm;smp-lapic,managed-exec-config, andsandbox-lifecycle;