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.

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

Flaky TestForDeps/loop-matrix test #1819

Closed
Ikke opened this issue Sep 19, 2024 · 5 comments · Fixed by #1839
Closed

Flaky TestForDeps/loop-matrix test #1819

Ikke opened this issue Sep 19, 2024 · 5 comments · Fixed by #1839
Labels
area: loops Changes related to looping over tasks/commands. type: bug Something not working as intended.

Comments

@Ikke
Copy link

Ikke commented Sep 19, 2024

  • Task version: 3.39.x
  • Operating system: Alpine Linux
  • Experiments enabled: None

I'm maintaining the Alpine Linux package for task. While upgrading to 3.39.1 and 3.39.2, the TestForDeps/loop-matrix failed occasionally with:

=== RUN   TestForDeps/loop-matrix
    task_test.go:2495: 
        	Error Trace:	github.com/go-task/task/v3/task_test.go:2495
        	Error:      	"linux/arm64\ndarwin/amd64\ndarwin/arm64\nwindows/amd64\nlinux/amd64windows/arm64\n\n" does not contain "linux/amd64\n"
        	Test:       	TestForDeps/loop-matrix

Retrying the build fixes it. This may indicate some kind of race condition. The failure did happen on somewhat slower arches (armv7, riscv64).

@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label Sep 19, 2024
@pd93
Copy link
Member

pd93 commented Sep 19, 2024

Strange, the test uses a SyncBuffer that should only be able to write one thing at a time, so it's odd that you're getting linux/amd64windows/arm64\n\n in your output instead of linux/amd64\nwindows/arm64\n. I suppose new newline is written separately somewhere. I'm guessing the quick fix is to remove the \n from the test, but this feels like sweeping the issue under the rug.

Do you know if this can happen in versions prior to 3.39.1 or was it introduced in that version?

@pd93 pd93 added type: bug Something not working as intended. area: loops Changes related to looping over tasks/commands. and removed state: needs triage Waiting to be triaged by a maintainer. labels Sep 19, 2024
@Ikke
Copy link
Author

Ikke commented Sep 19, 2024

On 3.38.0, running test with -count 100:

    task_test.go:2399:
                Error Trace:    /home/buildozer/aports/community/go-task/src/task-3.38.0/task_test.go:2399
                Error:          "21\n\n3\n" does not contain "2\n"
                Test:           TestForDeps/loop-different-tasks
--- FAIL: TestForDeps (0.08s)

On 3.37.0 additionally:

=== RUN   TestForDeps/loop-explicit
    task_test.go:2326:
                Error Trace:    /home/buildozer/aports/community/go-task/src/task-3.37.0/task_test.go:2326
                Error:          "ba\n\nc\n" does not contain "b\n"
                Test:           TestForDeps/loop-explicit

Different subtests, but similar issues. So not introduced in 3.39

@pd93
Copy link
Member

pd93 commented Sep 19, 2024

Thanks for checking. I will investigate

@pd93
Copy link
Member

pd93 commented Sep 20, 2024

Just leaving this as a note to myself. This just happened in our CI https://github.com/go-task/task/actions/runs/10962583048/job/30444328303

@pbitty
Copy link
Contributor

pbitty commented Sep 26, 2024

I have a clue as to why this might be happening. I added the following line inside SyncBuffer.Write() (inside the lock):

fmt.Fprintln(os.Stderr, "len:", len(p))

When I run the tests, I get this output:

go test -run TestForDeps/loop-explicit -count 1
len: 1
len: 1
len: 1
len: 1
len: 1
len: 1
PASS
ok  	github.com/go-task/task/v3	0.358s

This implies that each call to Write() contains a single character, so the lock won't prevent the task outputs from being interleaved, and explains why sometimes we get something like ba\n\nc\n.

pbitty added a commit to pbitty/go-task that referenced this issue Sep 26, 2024
The executor output for parallel task (and dep)
execution can come out one character at a time,
so even though the tests use `SyncWriter, the
writer's lock doesn't prevent interleaving.

Using `group` output forces the task output to be
buffered and kept together before being printed.

Fixes go-task#1819.
pbitty added a commit to Wattpad/go-task that referenced this issue Sep 26, 2024
The executor output for parallel task (and dep)
execution can come out one character at a time,
so even though the tests use `SyncWriter, the
writer's lock doesn't prevent interleaving.

Using `group` output forces the task output to be
buffered and kept together before being printed.

Fixes go-task#1819.
pbitty added a commit to Wattpad/go-task that referenced this issue Sep 26, 2024
The executor output for parallel task (and dep)
execution can come out one character at a time,
so even though the tests use `SyncWriter`, the
writer's lock doesn't prevent interleaving.

Using `group` output forces the task output to be
buffered and kept together before being printed.

Fixes go-task#1819.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: loops Changes related to looping over tasks/commands. type: bug Something not working as intended.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants