sql: fix TestCaptureIndexUsageStats #110639
Conversation
|
This change is |
maryliag
changed the title
There was a problem hiding this comment.
The time it takes is not really relevant for this test
nit / question: Can we just delete these assertions altogether? It seems obvious to expect something to take longer than 0 seconds. (As much as I wish that things could be instant 😄 )
Reviewable status:
complete! 1 of 0 LGTMs obtained
|
Why would it be equal to 0? Isn't that a bug as everything should take some amount of time. |
Tests can take longer to be completed, so a bigger time buffer would be required to accomodate for the non-determinism in the logging timings. This time buffer has been increasing every couple of months and it would have to change to 5s now, which is a big buffer compared to the actual value of 1s. The time it takes is not really relevant for this test, as long as the schedules are happening one after the other. So instead of increasing the time buffer even more, this commit removes those checks. This way the test is no longer flaky or would need to be adjusted every few months, and we keep all the actual checks important for the test. Fixes cockroachdb#110617 Release note: None
maryliag
force-pushed
the
increase-buffer
branch
from
f774b6d to
b45ba50
Compare
There was a problem hiding this comment.
Yeah, makes more sense to remove them, since they're not adding.
Reviewable status:
pkg/sql/scheduledlogging/captured_index_usage_stats_test.go line 282 at r1 (raw file):
Previously, j82w (Jake) wrote…
Why would it be equal to 0? Isn't that a bug as everything should take some amount of time.
I decided to remove those checks altogether, since they should always be greater than 0 anyway
|
bors r+ |
|
Build succeeded: |
Tests can take longer to be completed, so a bigger time buffer
would be required to accomodate for the non-determinism in
the logging timings. This time buffer has been increasing every
couple of months and it would have to change to 5s now, which is
a big buffer compared to the actual value of 1s. The time it takes
is not really relevant for this test, as long as the schedules
are happening one after the other.
So instead of increasing the time buffer even more, this commit
removes those checks. This way the test is no longer flaky or would need
to be adjusted every few months, and we keep all the actual checks
important for the test.
Fixes #110617
Release note: None