fix: return full path in getTaskPath() instead of bare filename - #2971
fix: return full path in getTaskPath() instead of bare filename#2971Nithwin wants to merge 1 commit into
Conversation
|
@trulede Could you give me your review please. |
trulede
left a comment
There was a problem hiding this comment.
I make a suggestion, but otherwise fine.
| func getTaskPath() (string, error) { | ||
| if info, err := os.Stat("./bin/task"); err == nil { | ||
| return info.Name(), nil | ||
| const localTaskBin = "./bin/task" |
There was a problem hiding this comment.
I suggest this, with associated changes:
var sleepit, _ = filepath.Abs("./bin/sleepit")
var localTaskBinary, _ = filepath.Abs("./bin/task")
...
func TestSignalSentToProcessGroup(t *testing.T) {
task := localTaskBinary
if task == "" {
// Fallback to system task if local build doesn't exist
var err error
task, err = exec.LookPath("task")
if err != nil {
t.Fatal(err)
}
}
// ... rest of test
}
There was a problem hiding this comment.
Thanks for the suggestion! 👍
I've implemented your approach and it's much better.
|
FYI - Using "Squash and merge" when merging this PR would ensure the contribution shows up in the contributors list. Thanks! |
| ) | ||
|
|
||
| var SLEEPIT, _ = filepath.Abs("./bin/sleepit") | ||
| var localTaskBinary, _ = filepath.Abs("./bin/task") |
There was a problem hiding this comment.
Fix sleepit too, style matters.
| task, err := getTaskPath() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| task := localTaskBinary |
There was a problem hiding this comment.
Now you don't check if the path exists? So the following code is not hit. Right?
This style is better when possible:
if task, err = exec.LookPath("task"); err != nil {
t.Fatal(err)
}
|
@trulede Thanks for catching that! You were totally right the path wasn't being checked for existence, so the fallback was never hit. I just pushed an update:
Thanks again for the help! |
trulede
left a comment
There was a problem hiding this comment.
I would suggest starting again. This is going nowhere.
Just fix the original problem.
0fdaafe to
dfa702a
Compare
|
What agent do you use to write this? I suggest to put the original code into google.ai and look at what it suggests, and then really think about which of those suggestions is the best. And more importantly, if they are better than what you currently have. |
|
I'm closing this PR as it doesn't follow our contribution guidelines. Please refer to our AI usage policy. |
|
Thank you so much for your feedback! It helped me realize that I have been overly dependent on AI. I will ensure to solve future problems myself. |
Summary
Fixes #2970
Problem
In
signals_test.go,getTaskPath()usesos.Stat("./bin/task")and returnsinfo.Name(). TheFileInfo.Name()method only returns the base filename ("task"), not the path passed toos.Stat("./bin/task").This means
exec.Commandreceives"task"(bare name) and resolves it via$PATH, silently running signal tests against a system-installed binary instead of the locally-built one.Fix
Store the path in a constant and return it directly:
func getTaskPath() (string, error) { - if info, err := os.Stat("./bin/task"); err == nil { - return info.Name(), nil + const localTaskBin = "./bin/task" + if _, err := os.Stat(localTaskBin); err == nil { + return localTaskBin, nil }Testing
signals_test.go)os.FileInfo.Name()only returns the base filename per the Go standard library docs