Fix bug in Start ctx handling

Options that had no context set were overwriting contexts that did. This both fixes that and tests to prevent regression. Signed-off-by: Ed Warnicke <[email protected]>
edwarnicke · May 19, 2020 · 8218f0e · 8218f0e
1 parent a22a20b
commit 8218f0e
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 4 deletions.
diff --git a/exechelper.go b/exechelper.go
@@ -37,7 +37,9 @@ func Start(cmdStr string, options ...*Option) <-chan error {
 	// Set the context
 	var ctx context.Context
 	for _, option := range options {
-		ctx = option.Context
+		if option.Context != nil {
+			ctx = option.Context
+		}
 	}
 
 	// Construct the command args

diff --git a/exechelper_test.go b/exechelper_test.go
@@ -13,6 +13,7 @@ import (
 	"path"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 
@@ -60,10 +61,17 @@ func TestCombinedOutputWithDir(t *testing.T) {
 
 func TestStartWithContext(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
-	errCh := exechelper.Start("sleep 600", exechelper.WithContext(ctx))
+	errCh := exechelper.Start("sleep 600", exechelper.WithContext(ctx), exechelper.CmdOption(func(cmd *exec.Cmd) error {
+		return nil
+	}))
 	cancel()
-	assert.IsType(t, &exec.ExitError{}, <-errCh) // Because we canceled we will get an exec.ExitError{}
-	assert.Empty(t, errCh)
+	select {
+	case err := <-errCh:
+		assert.IsType(t, &exec.ExitError{}, err) // Because we canceled we will get an exec.ExitError{}
+		assert.Empty(t, errCh)
+	case <-time.After(time.Second):
+		assert.Fail(t, "Failed to cancel context")
+	}
 }
 
 func TestCmdOptionErr(t *testing.T) {