Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,11 @@ func (e *Executor) ShellRun(ctx context.Context, command string, opts ...shell.R
}
options = append(options, e.shellRunOptions...)
options = append(options, opts...)
return shell.Run(ctx, command, options...)
err := shell.Run(ctx, command, options...)
if err != nil {
return oops.Wrapf(err, "Executor failed to run shell command")
}
return nil
}

func (e *Executor) taskExecution(t *Task) *taskExecution { return e.tasks[t] }
Expand Down Expand Up @@ -685,7 +689,7 @@ func (e *Executor) runPass() {

e.runPass()
if err != nil && ctx.Err() != context.Canceled {
return err
return oops.Wrapf(err, "Executor failed to run task")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try this out, and see what it looks like now when a task fails — preferably both in TUI mode and in non-TUI mode — just in case it turns out to be weird/bad/confusing in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this - this error will only show up in the non UI version of Taskrunner

github.com/samsarahq/taskrunner.(*Executor).runPass.(*Executor).runPass.func1.func2: Executor failed to run task "tr-error-test"
        github.com/taskrunner/executor.go:648
golang.org/x/sync/errgroup.(*Group).Go.func1
        /home/ubuntu/co/backend/go/pkg/mod/golang.org/x/sync@v0.0.0-20200317015054-43a5402ce75a/errgroup/errgroup.go:57

github.com/samsarahq/taskrunner.Run.func4: running executor

}

return nil
Expand Down
61 changes: 61 additions & 0 deletions executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package taskrunner_test

import (
"context"
"errors"
"testing"
"time"

Expand Down Expand Up @@ -194,3 +195,63 @@ func TestExecutorInvalidations(t *testing.T) {
mockB.Reset()
}
}

func TestExecutorErrorHandling(t *testing.T) {
config := &config.Config{}

for _, testcase := range []struct {
Name string
Test func(t *testing.T)
}{
{
"shell command error",
func(t *testing.T) {
executor := taskrunner.NewExecutor(config, []*taskrunner.Task{
{
Name: "shell-error",
Run: func(ctx context.Context, shellRun shell.ShellRun) error {
return shellRun(ctx, "invalid_command_that_will_fail")
},
},
})

events := executor.Subscribe()
go func() {
event := consumeUntil(t, events, taskrunner.ExecutorEventKind_TaskFailed)
assert.Contains(t, event.(*taskrunner.TaskFailedEvent).Error.Error(),
"Executor failed to run shell command")
}()

err := executor.Run(context.Background(), []string{"shell-error"}, &taskrunner.Runtime{})
assert.Error(t, err)
assert.Contains(t, err.Error(), "Executor failed to run task")
},
},
{
"task execution error",
func(t *testing.T) {
executor := taskrunner.NewExecutor(config, []*taskrunner.Task{
{
Name: "failing-task",
Run: func(ctx context.Context, shellRun shell.ShellRun) error {
return errors.New("task failed")
},
},
})

events := executor.Subscribe()
go func() {
event := consumeUntil(t, events, taskrunner.ExecutorEventKind_TaskFailed)
assert.Contains(t, event.(*taskrunner.TaskFailedEvent).Error.Error(),
"task failed")
}()

err := executor.Run(context.Background(), []string{"failing-task"}, &taskrunner.Runtime{})
assert.Error(t, err)
assert.Contains(t, err.Error(), "Executor failed to run task")
},
},
} {
t.Run(testcase.Name, testcase.Test)
}
}
7 changes: 5 additions & 2 deletions runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,10 @@ func Run(options ...RunOption) {
sub := runtime.subscriptions[i]
ch := executor.Subscribe()
g.Go(func() error {
return sub(ch)
if err := sub(ch); err != nil {
return oops.Wrapf(err, "subscription error")
}
return nil
})
}

Expand All @@ -264,7 +267,7 @@ func Run(options ...RunOption) {
// if it's a well-known executor error, or the underlying task failed AND
// we're not in watch mode.
if oops.Cause(err) == errUndefinedTaskName || !watchMode {
return err
return oops.Wrapf(err, "running executor")
}

return nil
Expand Down
3 changes: 2 additions & 1 deletion shell/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"io"
"strings"

"github.com/samsarahq/go/oops"
"mvdan.cc/sh/interp"
"mvdan.cc/sh/syntax"
)
Expand Down Expand Up @@ -48,7 +49,7 @@ func Env(vars map[string]string) RunOption {
func Run(ctx context.Context, command string, opts ...RunOption) error {
p, err := syntax.NewParser().Parse(strings.NewReader(command), "")
if err != nil {
return err
return oops.Wrapf(err, "failed to parse shell command")
}

r, err := interp.New()
Expand Down
17 changes: 17 additions & 0 deletions shell/shell_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,20 @@ func TestRunShell(t *testing.T) {
assert.NoError(t, Run(context.Background(), "echo 1", Stdout(&buffer)))
assert.Equal(t, "1\n", buffer.String())
}

func TestRunShellInterpreterError(t *testing.T) {
defer func() {
if r := recover(); r != nil {
err, ok := r.(error)
assert.True(t, ok)
assert.Contains(t, err.Error(), "failed to set up interpreter")
}
}()
Run(context.Background(), "")
}

func TestRunShellParseError(t *testing.T) {
err := Run(context.Background(), "`invalid command")
assert.Error(t, err)
assert.Contains(t, err.Error(), "failed to parse shell command")
}