Skip to content
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.

Sign up for GitHub

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

Fix bug for running tasks twice #1804

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix bug for running tasks twice #1804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5163578
Fix bug for running tasks twice
libre-man Sep 12, 2024
3ecbc01
Fix race conditions in test file
libre-man Sep 12, 2024
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
38 changes: 25 additions & 13 deletions task_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ type SyncBuffer struct {
mu sync.Mutex
}

func (sb *SyncBuffer) String() string {
sb.mu.Lock()
defer sb.mu.Unlock()
return sb.buf.String()
}

func (sb *SyncBuffer) Reset() {
sb.mu.Lock()
defer sb.mu.Unlock()
sb.buf.Reset()
}

Comment on lines +40 to +51
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are necessary. You can just call buff.buf.Reset() like you have done elsewhere. The only operation that needs a mutex is Write().

func (sb *SyncBuffer) Write(p []byte) (n int, err error) {
sb.mu.Lock()
defer sb.mu.Unlock()
Expand Down Expand Up @@ -179,7 +191,7 @@ func TestSpecialVars(t *testing.T) {
for _, dir := range []string{dir, subdir} {
for _, test := range tests {
t.Run(test.target, func(t *testing.T) {
var buff bytes.Buffer
var buff SyncBuffer
e := &task.Executor{
Dir: dir,
Stdout: &buff,
Expand All @@ -188,7 +200,7 @@ func TestSpecialVars(t *testing.T) {
}
require.NoError(t, e.Setup())
require.NoError(t, e.Run(context.Background(), &ast.Call{Task: test.target}))
assert.Equal(t, test.expected+"\n", buff.String())
assert.Equal(t, test.expected+"\n", buff.buf.String())
})
}
}
Expand Down Expand Up @@ -286,7 +298,7 @@ func TestStatus(t *testing.T) {
}
}

var buff bytes.Buffer
var buff SyncBuffer
e := &task.Executor{
Dir: dir,
TempDir: task.TempDir{
Expand Down Expand Up @@ -322,7 +334,7 @@ func TestStatus(t *testing.T) {
require.NoError(t, e.Run(context.Background(), &ast.Call{Task: "gen-bar"}))

// We're silent, so no output should have been produced.
assert.Empty(t, buff.String())
assert.Empty(t, buff.buf.String())

// Now, let's remove source file, and run the task again to to prepare
// for the next test.
Expand Down Expand Up @@ -371,7 +383,7 @@ func TestStatus(t *testing.T) {
func TestPrecondition(t *testing.T) {
const dir = "testdata/precondition"

var buff bytes.Buffer
var buff SyncBuffer
e := &task.Executor{
Dir: dir,
Stdout: &buff,
Expand Down Expand Up @@ -486,7 +498,7 @@ func TestStatusChecksum(t *testing.T) {
require.Error(t, err)
}

var buff bytes.Buffer
var buff SyncBuffer
tempdir := task.TempDir{
Remote: filepathext.SmartJoin(dir, ".task"),
Fingerprint: filepathext.SmartJoin(dir, ".task"),
Expand Down Expand Up @@ -1735,7 +1747,7 @@ func TestRunOnlyRunsJobsHashOnce(t *testing.T) {
func TestRunOnceSharedDeps(t *testing.T) {
const dir = "testdata/run_once_shared_deps"

var buff bytes.Buffer
var buff SyncBuffer
e := task.Executor{
Dir: dir,
Stdout: &buff,
Expand All @@ -1746,10 +1758,10 @@ func TestRunOnceSharedDeps(t *testing.T) {
require.NoError(t, e.Run(context.Background(), &ast.Call{Task: "build"}))

rx := regexp.MustCompile(`task: \[service-[a,b]:library:build\] echo "build library"`)
matches := rx.FindAllStringSubmatch(buff.String(), -1)
matches := rx.FindAllStringSubmatch(buff.buf.String(), -1)
assert.Len(t, matches, 1)
assert.Contains(t, buff.String(), `task: [service-a:build] echo "build a"`)
assert.Contains(t, buff.String(), `task: [service-b:build] echo "build b"`)
assert.Contains(t, buff.buf.String(), `task: [service-a:build] echo "build a"`)
assert.Contains(t, buff.buf.String(), `task: [service-b:build] echo "build b"`)
}

func TestDeferredCmds(t *testing.T) {
Expand Down Expand Up @@ -2412,8 +2424,8 @@ func TestForCmds(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
var stdOut bytes.Buffer
var stdErr bytes.Buffer
var stdOut SyncBuffer
var stdErr SyncBuffer
e := task.Executor{
Dir: "testdata/for/cmds",
Stdout: &stdOut,
Expand All @@ -2423,7 +2435,7 @@ func TestForCmds(t *testing.T) {
}
require.NoError(t, e.Setup())
require.NoError(t, e.Run(context.Background(), &ast.Call{Task: test.name}))
assert.Equal(t, test.expectedOutput, stdOut.String())
assert.Equal(t, test.expectedOutput, stdOut.buf.String())
})
}
}
Expand Down
10 changes: 6 additions & 4 deletions taskfile/ast/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ type Task struct {
Watch bool
Location *Location
// Populated during merging
Namespace string
Namespace []string
IncludeVars *Vars
IncludedTaskfileVars *Vars
}
Expand All @@ -57,8 +57,10 @@ func (t *Task) Name() string {

func (t *Task) LocalName() string {
name := t.Task
name = strings.TrimPrefix(name, t.Namespace)
name = strings.TrimPrefix(name, ":")
for _, namespace := range t.Namespace {
name = strings.TrimPrefix(name, namespace)
name = strings.TrimPrefix(name, ":")
}
return name
}

Expand Down Expand Up @@ -219,7 +221,7 @@ func (t *Task) DeepCopy() *Task {
Platforms: deepcopy.Slice(t.Platforms),
Location: t.Location.DeepCopy(),
Requires: t.Requires.DeepCopy(),
Namespace: t.Namespace,
Namespace: append([]string{}, t.Namespace...),
Copy link
Member

Choose a reason for hiding this comment

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

This change isn't necessary.

Suggested change
Namespace: append([]string{}, t.Namespace...),
Namespace: t.Namespace,

Copy link
Author

Choose a reason for hiding this comment

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

But this is not an array, which would mean that a deep copy would need to deep copy the array too right?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes sorry, you are right. We have deepcopy.Slice() for this.

Suggested change
Namespace: append([]string{}, t.Namespace...),
Namespace: deepcopy.Slice(t.Namespace),

}
return c
}
2 changes: 1 addition & 1 deletion taskfile/ast/tasks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func (t1 *Tasks) Merge(t2 Tasks, include *Include, includedTaskfileVars *Vars) e
}

taskName = taskNameWithNamespace(name, include.Namespace)
task.Namespace = include.Namespace
task.Namespace = append([]string{include.Namespace}, task.Namespace...)
task.Task = taskName
}

Expand Down
Loading