Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karampok)

go/lib/periodic/periodic_test.go, line 73 at r1 (raw file):

func TestKillExitsLongRunningFunc(t *testing.T) {
	t.Parallel()

why?

go/lib/periodic/periodic_test.go, line 77 at r1 (raw file):

	p := 10 * time.Millisecond
	fn := taskFunc(func(ctx context.Context) {
		done <- struct{}{}

why not simply close(done) ?

go/lib/periodic/periodic_test.go, line 86 at r1 (raw file):

		errChan <- ctx.Err()
	})
	r := Start(fn, p, 2*p)

I would make the timeout larger here (infinite, e.g. 1 hour), just to make sure the kill actually cancelled the context and not the timeout.

go/lib/periodic/periodic_test.go, line 99 at r1 (raw file):

func TestTaskDoesntRunAfterKill(t *testing.T) {
	t.Parallel()

needed?

go/lib/periodic/periodic_test.go, line 123 at r1 (raw file):

func TestTriggerNow(t *testing.T) {
	t.Parallel()

?

go/lib/periodic/periodic_test.go, line 155 at r1 (raw file):

}

func waitWithTimeout(ch <-chan struct{}, d time.Duration) error {

We have xtest.AssertReadReturnsBefore that does this, why not use that?

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karampok and @lukedirtwalker)

go/lib/periodic/periodic_test.go, line 73 at r1 (raw file):

I was not really planned it to commit that.
Nevertheless, running test in parallel usually reveals issues, do you (or we) have a policy not to allow tests in parallel?

go/lib/periodic/periodic_test.go, line 77 at r1 (raw file):

I will do that, but what is the benefit of close over just write?

go/lib/periodic/periodic_test.go, line 99 at r1 (raw file):

Done.

go/lib/periodic/periodic_test.go, line 123 at r1 (raw file):

Done.

go/lib/periodic/periodic_test.go, line 155 at r1 (raw file):

My preference is not to create a dependency in a lib for a 5 lines function.
Nevertheless no strong opinion

Reviewed 3 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved