-
Notifications
You must be signed in to change notification settings - Fork 647
fix: ensure usrlocalsharelima.Dir() works when called from tests #3401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There is something wrong with this PR; it adds almost 2000 lines, many unrelated to the issue it is supposed to fix. This may be responsible for the various test failures. Don't merge Please squash commits into a single commit and add a DCO signature. Thanks! |
@Konikz Are you planning to fix this PR? There is nothing we can do with it until the extraneous commits are removed and the lint failures addressed. |
@jandubois I'll rectify the issue with the pr |
76f3619
to
f0883e9
Compare
216015b
to
4c82f35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
I would change the logic to use Must()
, but feel free to ignore if you like your approach better.
However, you do need to squash commits before we can merge.
@@ -16,8 +16,11 @@ import ( | |||
"github.com/sirupsen/logrus" | |||
) | |||
|
|||
// executable is a variable that can be overridden in tests. | |||
var executable = os.Executable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just cache the return value of the function because it is constant over the lifetime of the process. Import Must
at the top:
import (
…
. "github.com/lima-vm/lima/pkg/must"
)
and then use
var executable = os.Executable | |
var self = Must(os.Executable()) |
later you can simply assign to it:
self = limactlPath
See osutil/user.go
for an example of using Must()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sir @jandubois ,
I have tried implementing the above suggestions, I need your suggestion with the failing tests.
13e5ad3
to
a69b55d
Compare
16b393d
to
abf44b3
Compare
This commit implements the following improvements:\n- Caches the executable path at package level using Must()\n- Makes the Dir() function testable\n- Adds a test to verify functionality Signed-off-by: Konikz <[email protected]>
abf44b3
to
c390ecc
Compare
Fixes #3208
During tests,
usrlocalsharelima.Dir()
fails to find the guestagent binary because the test executable is in a temp directory. This PR adds a test that verifies the binary can be found in both expected locations:share/lima
directory relative to the test executableThe test creates dummy binaries in both locations and verifies
Dir()
can find them. This ensures the function works correctly in test environments whereos.Executable
returns a temp directory path.