healthpool: fix flaky test #3307
Conversation
c600756 to
3d58fe7
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @karampok and @oncilla)
a discussion (no related file):
go test -v -run TestPoolExpiresFails -race -count 3
=== RUN TestPoolExpiresFails
INFO[11-05|10:19:37] Starting periodic task debug_id=9acb0442 task=healthpool_expirer
--- FAIL: TestPoolExpiresFails (0.00s)
<autogenerated>:1:
Error Trace: pool_test.go:163
Error: Not equal:
expected: 16
actual : 64
Test: TestPoolExpiresFails
=== RUN TestPoolExpiresFails
INFO[11-05|10:19:37] Starting periodic task debug_id=f0c5341e task=healthpool_expirer
--- PASS: TestPoolExpiresFails (0.00s)
=== RUN TestPoolExpiresFails
INFO[11-05|10:19:37] Starting periodic task debug_id=aa209b8e task=healthpool_expirer
--- FAIL: TestPoolExpiresFails (0.00s)
<autogenerated>:1:
Error Trace: pool_test.go:163
Error: Not equal:
expected: 16
actual : 64
Test: TestPoolExpiresFails
FAIL
exit status 1
FAIL github.com/scionproto/scion/go/lib/healthpool 0.012s
go/lib/healthpool/pool_test.go, line 55 at r1 (raw file):
for name, test := range tests { t.Run(name, func(t *testing.T) {
I would like to start having a t.parallel() in all table-drive tests
go/lib/healthpool/pool_test.go, line 136 at r1 (raw file):
} func TestPoolExpiresFails(t *testing.T) {
fails with the -race see a discussion
go/lib/healthpool/pool_test.go, line 157 at r1 (raw file):
Expire: ExpireOptions{ Interval: time.Hour / 2, Start: time.Nanosecond,
There should be a tiny bit in description about that change
go/lib/healthpool/pool_test.go, line 175 at r1 (raw file):
expected = append(expected, info) } assert.ElementsMatch(t, expected, pool)
either you have to use t.Helper() or to return an error and do the assert in the main code (the latter is my preference)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @oncilla)
3d58fe7 to
a709dde
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karampok)
a discussion (no related file):
Previously, karampok (Konstantinos) wrote…
go test -v -run TestPoolExpiresFails -race -count 3 === RUN TestPoolExpiresFails INFO[11-05|10:19:37] Starting periodic task debug_id=9acb0442 task=healthpool_expirer --- FAIL: TestPoolExpiresFails (0.00s) <autogenerated>:1: Error Trace: pool_test.go:163 Error: Not equal: expected: 16 actual : 64 Test: TestPoolExpiresFails === RUN TestPoolExpiresFails INFO[11-05|10:19:37] Starting periodic task debug_id=f0c5341e task=healthpool_expirer --- PASS: TestPoolExpiresFails (0.00s) === RUN TestPoolExpiresFails INFO[11-05|10:19:37] Starting periodic task debug_id=aa209b8e task=healthpool_expirer --- FAIL: TestPoolExpiresFails (0.00s) <autogenerated>:1: Error Trace: pool_test.go:163 Error: Not equal: expected: 16 actual : 64 Test: TestPoolExpiresFails FAIL exit status 1 FAIL github.com/scionproto/scion/go/lib/healthpool 0.012s
argh, should be fixed now.
go/lib/healthpool/pool_test.go, line 55 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
I would like to start having a
t.parallel()in all table-drive tests
Done.
go/lib/healthpool/pool_test.go, line 136 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
fails with the
-racesee a discussion
Done.
go/lib/healthpool/pool_test.go, line 157 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
There should be a tiny bit in description about that change
ha, that line was not actually the problem.
The problem was, that the async periodic task that decreases the fail count did not run before the asserts.
Added sleep now after TriggerRun to ensure the Task is run at least once.
go/lib/healthpool/pool_test.go, line 175 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
either you have to use t.Helper() or to return an error and do the assert in the main code (the latter is my preference)
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @karampok and @oncilla)
go/lib/healthpool/pool_test.go, line 55 at r1 (raw file):
Previously, Oncilla wrote…
Done.
@@ -54,6 +55,7 @@ func TestNewPool(t *testing.T) {
for name, test := range tests {
t.Run(name, func(t *testing.T) {
t.Parallel()
+ fmt.Println(name)
p, err := NewPool(test.Infos, test.Options)
test.Assertion(t, err)
+✔ ~/go/src/github.com/scionproto/scion/go/lib/healthpool [pr/3307 L|✚ 1…4⚑ 2]
13:22 $ go test -run NewPool
invalid algorithm
invalid algorithm
invalid algorithm
invalid algorithm
you need to scope out the name,test, it is a bit sneaky the the t.parallel()
go/lib/healthpool/pool_test.go, line 164 at r2 (raw file):
require.NoError(t, err) p.expirer.TriggerRun() time.Sleep(time.Second)
one full second?! I have the feeling that we need to change the TriggerRun() to block until a run finishes or optionally
return a channel so to indicate when finish.
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @oncilla)
a709dde to
05a5ea9
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @karampok and @oncilla)
go/lib/healthpool/pool_test.go, line 55 at r1 (raw file):
Previously, karampok (Konstantinos) wrote…
@@ -54,6 +55,7 @@ func TestNewPool(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { t.Parallel() + fmt.Println(name) p, err := NewPool(test.Infos, test.Options) test.Assertion(t, err)+✔ ~/go/src/github.com/scionproto/scion/go/lib/healthpool [pr/3307 L|✚ 1…4⚑ 2] 13:22 $ go test -run NewPool invalid algorithm invalid algorithm invalid algorithm invalid algorithmyou need to scope out the
name,test, it is a bit sneaky the thet.parallel()
ugh, good to know.
go/lib/healthpool/pool_test.go, line 164 at r2 (raw file):
Previously, karampok (Konstantinos) wrote…
one full second?! I have the feeling that we need to change the TriggerRun() to block until a run finishes or optionally
return a channel so to indicate when finish.
Trigger run blocks until a new run is started.
However, there is no way to detect whether the run has finished.
This would be a major change, and should not be part of this PR.
One second is huge, yes, but a safe value. Okay let's go with 1 millisecond.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
05a5ea9 to
60f29c7
Compare
The TestPoolExpiresFails test no longer is flaky.
Additionally, remove goconvey.
This change is