pkg/expect: avoid hardcoding when checking ErrProcessDone #16252
Conversation
pkg/expect/expect.go
Outdated
| @@ -286,7 +286,7 @@ func (ep *ExpectProcess) ExitError() error { | |||
| // Stop signals the process to terminate via SIGTERM | |||
| func (ep *ExpectProcess) Stop() error { | |||
| err := ep.Signal(syscall.SIGTERM) | |||
| if err != nil && strings.Contains(err.Error(), "os: process already finished") { | |||
| if err == os.ErrProcessDone { | |||
There was a problem hiding this comment.
Should we use if err != nil && errors.Is(err, os.ErrProcessDone) here? I think it is easy to read and it's better to check err compatible with Is instead of equal. WDYT?
There was a problem hiding this comment.
To be clear, I don't want to abuse 'errors.Is'. However It's harmless, I'll change it.
There was a problem hiding this comment.
Thanks for the update. For the errors.Is, since the function or interface might enhance the error message with fmt.Errorf, it's hard to track the change. It might happen that the err is not os.ErrProcessDone but the main cause it is. So, it's just my two cents.
There was a problem hiding this comment.
Thanks, I understand.
I also noticed this code go/src/cmd/go/internal/vcweb/hg.go at line 62.
if err != nil && !errors.Is(err, os.ErrProcessDone)
ExpectProcess's Stop method uses 'strings.Contains' to check the returned err, however, this can be avoided. os.ErrProcessDone's error message is the same as the hardcoded string. So I think this explicit error is what this method wants to compare. Signed-off-by: Jes Cok <xigua67damn@gmail.com>
ExpectProcess's Stop method uses 'strings.Contains' to check the returned err, however, this can be avoided. os.ErrProcessDone's error message is the same as the hardcoded string. So I think this explicit error is what this method wants to compare.