Fixing matching:TestCheckIdleTaskList test flackiness #5494
Conversation
dkrotx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
taylanisikdemir and
demirkayaender
as code owners
| // mimic tlm.Start() but avoid calling notifyEvent | ||
| tlm.liveness.Start() | ||
| tlm.startWG.Done() | ||
| go tlm.taskReader.dispatchBufferedTasks(defaultTaskBufferIsolationGroup) |
There was a problem hiding this comment.
sorry, small brain following along here, are we ensuring these daemons are started in the new test?
I 100% agree these tests are flakey and need some refactoring, but I'd also hate for us to lose testing of functionality, so it might just take me a minute to ensure they're equivalent
There was a problem hiding this comment.
I looked at the code - these daemons are starting among with tlm.taskReader.Start():
for g := range tr.taskBuffers {
go tr.dispatchBufferedTasks(g)
}
go tr.getTasksPump()
I think the previous approach where we started selected things bypassing the tlm methods actually a bad approach. We were trying to avoid notifyEvent (which is, I believe, no longer happening in .Start), but also missed some important changes like starting the writer.
I think it is really important for us to cover the happy-paths as well. Otherwise tests like TestAddTaskStandby pass unintentionally: it assumed we had to stop the writer though it's never been started!
For some reason it almost always fails in IDE (4/5 cases, goland), but passes in console. At the same time, the test clearly had issues: - it's been 3 subtests in one - only the first subtest checked task-list became stopped when unused - the third subtest (the one which has been failing) never succeeded with `tlm.AddTask` since writer never started - instead of tlm.Start() we used special version which supposed to mimic it, but never started the writer. The test is still problematic since it continues using time.Sleep all around, but so do all the other tests for matching :-( More comments and error-messages also bring more clarity to what the test is actually for.
dkrotx
force-pushed
the
fix-matching-test-flakiness
branch
from
c7d4ed1 to
efa4696
Compare
For some reason it almost always fails in IDE (4/5 cases, goland),
but passes in console. At the same time, the test clearly had issues:
with
tlm.AddTasksince writer never started - instead of tlm.Start()we used special version which supposed to mimic it, but never started
the writer.
The test is still problematic since it continues using time.Sleep all
around, but so do all the other tests for matching :-(
More comments and error-messages also bring more clarity to what the
test is actually for.
fix test flackiness in IDE
To be able to run tests in GoLand
make test + running all tests in GoLand
No risks, it is only the test been improved
Release notes
Documentation Changes