Add unit tests for VM Driver Registry package #3735
Conversation
unsuman
force-pushed
the
test/registry
branch
2 times, most recently
from
00bce5e to
f7dd1a4
Compare
| @@ -177,7 +177,7 @@ func registerDriverFile(path string) { | |||
| name = strings.TrimSuffix(strings.TrimPrefix(base, "lima-driver-"), ".exe") | |||
| } | |||
| } else { | |||
| if strings.HasPrefix(base, "lima-driver-") { | |||
| if strings.HasPrefix(base, "lima-driver-") && !strings.HasSuffix(base, ".exe") { | |||
There was a problem hiding this comment.
May I suggest creating a separate PR for this fix?
There was a problem hiding this comment.
Can be also a separate commit in the same PR
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests for the VM driver registry package and fixes a platform-specific bug. The tests validate the core functionality of driver registration and discovery, while the bug fix ensures that .exe files are properly filtered on non-Windows platforms.
- Added comprehensive unit tests covering internal driver registration, external driver discovery, and driver retrieval functions
- Fixed bug preventing
.exefiles from being incorrectly registered as drivers on non-Windows platforms
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/registry/registry_test.go | Added comprehensive unit tests for registry functionality including mock driver implementation and test cases for all major functions |
| pkg/registry/registry.go | Fixed platform-specific bug to prevent .exe files from being registered as drivers on non-Windows systems |
Comments suppressed due to low confidence (3)
pkg/registry/registry_test.go:55
- The test directly accesses the global internalDrivers map but doesn't clean it up between tests. This could cause test interdependencies and flaky behavior. Consider resetting the global state in each test or using a setup/teardown mechanism.
assert.Equal(t, len(internalDrivers), 2)
pkg/registry/registry_test.go:103
- The test accesses the global ExternalDrivers map without cleaning it up. This could cause test pollution and unpredictable results when tests run in different orders. The cleanup should reset ExternalDrivers to its initial state.
assert.Equal(t, len(ExternalDrivers), 1)
pkg/registry/registry_test.go:177
- Global state is being reset in TestGet but not cleaned up afterwards. This could affect other tests that run after this one. Add t.Cleanup() to restore the original state or use a test setup/teardown pattern.
internalDrivers = make(map[string]driver.Driver)
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ExternalDrivers = make(map[string]*ExternalDriver) |
There was a problem hiding this comment.
[nitpick] Resetting global state within individual test cases can make tests harder to understand and maintain. Consider moving this setup to a helper function or using t.Cleanup() to ensure proper teardown regardless of test outcome.
| ExternalDrivers = make(map[string]*ExternalDriver) | |
| setupExternalDrivers(t) |
Copilot uses AI. Check for mistakes.
|
Looks like these Copilot comments makes sense. pkg/registry/registry_test.go:55 The test directly accesses the global internalDrivers map but doesn't clean it up between tests. This could cause test interdependencies and flaky behavior. Consider resetting the global state in each test or using a setup/teardown mechanism. assert.Equal(t, len(internalDrivers), 2)pkg/registry/registry_test.go:103 The test accesses the global ExternalDrivers map without cleaning it up. This could cause test pollution and unpredictable results when tests run in different orders. The cleanup should reset ExternalDrivers to its initial state. assert.Equal(t, len(ExternalDrivers), 1)pkg/registry/registry_test.go:177 Global state is being reset in TestGet but not cleaned up afterwards. This could affect other tests that run after this one. Add t.Cleanup() to restore the original state or use a test setup/teardown pattern. internalDrivers = make(map[string]driver.Driver) |
pkg/registry/registry_test.go
Outdated
| t.Cleanup(func() { | ||
| err := os.Remove(driverPath) | ||
| assert.NilError(t, err) | ||
|
|
||
| _, err = os.Stat(driverPath) | ||
| assert.Assert(t, os.IsNotExist(err)) | ||
| }) |
There was a problem hiding this comment.
This is not needed since t.TempDir() automatically removes a directory on Cleanup.
pkg/registry/registry_test.go
Outdated
| originalExternalDrivers := ExternalDrivers | ||
| originalInternalDrivers := internalDrivers | ||
| t.Cleanup(func() { | ||
| ExternalDrivers = originalExternalDrivers | ||
| internalDrivers = originalInternalDrivers | ||
| }) |
There was a problem hiding this comment.
I propose creating a helper function and use it in all tests.
pkg/registry/registry_test.go
Outdated
|
|
||
| "gotest.tools/v3/assert" | ||
|
|
||
| "github.com/lima-vm/lima/pkg/driver" |
There was a problem hiding this comment.
| "github.com/lima-vm/lima/pkg/driver" | |
| "github.com/lima-vm/lima/v2/pkg/driver" |
Signed-off-by: Ansuman Sahoo <[email protected]>
Signed-off-by: Ansuman Sahoo <[email protected]>
This PR adds unit tests for the
registrypackage.Changes
pkg/registry/registry_test.gowith unit tests covering core functions like registering internal and external drivers, registering external driver executablespkg/registry/registry.go, which was causing.exefiles to be registered on non-Windows platform.Closes #3718