service: add unit tests for LCOW v2 shim service layer#2649
Open
shreyanshjain7174 wants to merge 3 commits intomicrosoft:mainfrom
Open
service: add unit tests for LCOW v2 shim service layer#2649shreyanshjain7174 wants to merge 3 commits intomicrosoft:mainfrom
shreyanshjain7174 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
shreyanshjain7174
force-pushed
the
service/unit-tests
branch
4 times, most recently
from
72ac4ca to
c703ec9
Compare
shreyanshjain7174
force-pushed
the
service/unit-tests
branch
from
c703ec9 to
d0fa692
Compare
shreyanshjain7174
force-pushed
the
service/unit-tests
branch
2 times, most recently
from
6612341 to
5315f01
Compare
jterry75
reviewed
Apr 28, 2026
jterry75
requested changes
Apr 28, 2026
Defines a narrow vmController interface in the service package and threads it through Service in place of the concrete *vm.Controller field. Production passes vm.New(); tests pass a generated gomock. The interface includes the methods pod.New requires (Guest, SCSIController, VPCIController, Plan9Controller, NetworkController) so the same field satisfies both Service's direct calls and pod.New's narrower interface via Go structural typing. Tests cover the production failure paths through the service RPC surface: Sandbox API (23 tests): - duplicate Create rejection, missing config.json, VM create / start failure - sandbox-id mismatch guards - stop success / failure / idempotency - wait clean / error / wait-failure / exit-status-failure exits - status mapping for every state, ping not-implemented - shutdown idempotency, running-VM termination, terminate-error swallowed - metrics success and stats failure Shimdiag API (10 tests): - pid passthrough, exec-in-host success / failure - tasks / share / stacks state guards - share missing host-path validation - stacks dump-failure and success Task API (16 tests): - consolidated state-guard and unknown-container guard tables - not-implemented surfaces, shutdown no-op - update validation: nil resources rejected, dispatch by resource type, per-resource failure surfaces - enrichNotFoundError pass-through and ErrNotFound preservation Mocks committed at mocks/mock_service.go with the standard build tag (windows && lcow). Standardising mock generation across controllers is tracked in microsoft#2707. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
shreyanshjain7174
force-pushed
the
service/unit-tests
branch
from
affc35e to
0f930d4
Compare
rawahars
reviewed
Apr 30, 2026
rawahars
reviewed
Apr 30, 2026
rawahars
reviewed
Apr 30, 2026
rawahars
reviewed
Apr 30, 2026
…rt grouping Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
marma-dev
approved these changes
May 4, 2026
| svc, _ := newTestService(t) | ||
|
|
||
| _, err := svc.pingSandboxInternal(context.Background(), &sandboxsvc.PingRequest{}) | ||
| if err == nil { |
Contributor
There was a problem hiding this comment.
Let's check specifically for errdefs.ErrNotImplemented.
The test only checks err == nil. If a future refactor returned something like errors.New("ping not supported"), this test would still pass while breaking the documented contract.
Suggestion:
if !errors.Is(err, errdefs.ErrNotImplemented) { t.Errorf("expected ErrNotImplemented, got %v", err) }
Contributor
Author
There was a problem hiding this comment.
Fixed in 2729c12 - now uses errors.Is(err, errdefs.ErrNotImplemented).
| svc, mockCtrl := newTestService(t) | ||
| mockCtrl.EXPECT().State().Return(vm.StateRunning) | ||
|
|
||
| missing := "Q:\\does-not-exist-" + t.Name() |
Contributor
There was a problem hiding this comment.
Nit: We could use t.TempDir() instead.
t.TempDir() is freshly created and empty, so the joined path is guaranteed not to exist, and the test stays portable to any drive layout.
e.g.
missing := filepath.Join(t.TempDir(), "does-not-exist-"+t.Name())
Contributor
Author
There was a problem hiding this comment.
Fixed in 2729c12 - uses filepath.Join(t.TempDir(), ...) now.
…empDir() for missing-path test Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
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
5 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.
Adds unit tests for the LCOW v2 shim service layer. The service orchestrates sandbox lifecycle (create / start / stop / wait / shutdown / status / metrics) by delegating to
vm.Controller; without these tests every code change had to be validated against a live HCS environment.Design
The
Servicestruct holds a singlevmControllerinterface field (defined intypes.go). Production passes*vm.Controller; tests pass a generated mock. The interface is a superset that includes the methodspod.Newneeds (Guest(),SCSIController(),Plan9Controller(),NetworkController()etc.) so the same field satisfies bothService's direct calls andpod.New's narrower interface via Go structural typing - no concrete-pointer leakage. This mirrors the mergedinternal/controller/podpattern.Coverage
49 tests across three files matching the production layout:
service_sandbox_internal_test.go(23): duplicate sandbox rejection, missingconfig.json, VM create / start failure, sandbox-id mismatch guards, stop success / failure / idempotency, wait clean / error / wait-failure / exit-status-failure exits, status mapping for every state, ping not-implemented, shutdown idempotency / running-VM termination / terminate-error swallowed, metrics success and stats failure.service_shimdiag_internal_test.go(10): pid passthrough, exec-in-host success / failure, tasks / share / stacks state guards, share missing host-path validation, stacks dump-failure and success.service_task_internal_test.go(16): consolidated state-guard and unknown-container guard tables, not-implemented surfaces, shutdown no-op, update validation (nil resources rejected, dispatch by resource type, per-resource failure surfaces),enrichNotFoundErrorpass-through andErrNotFoundpreservation.Out of scope
CreateSandbox->BuildSandboxConfig->CreateVMpath is exercised byinternal/builder/vm/lcow/tests and thetest/parity/vm/parity suite; the service-level surface is the orchestration logic only.StartSandboxpath reads boot files from disk; belongs infunctionaltagged tests.Mocks
mocks/mock_service.gois checked in. Standardising mock generation (directive in test file + CI drift validation) is tracked in #2707.