Do not Errorf during file watcher verification test loop. #3938
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ldez
added
status/2-needs-review
area/provider/file
bot/no-merge
and removed
status/0-needs-triage
labels
|
@timoreimann Can you rebase on 1.7? Better tests in stable is a good thing ;) |
timoreimann
force-pushed
the
dont-t-error-in-file-test-watcher-loop
branch
from
946ec7e
to
485aeed
Compare
|
@dtomcej done! |
The loop in TestProvideWithWatch is meant to be executed multiple times until either all configuration elements are found or the timeout expires. By calling assert.*, however, an assertion failure in a non-terminal loop run will cause t.Error to be invoked, thereby marking the whole test as failed even though subsequent loop runs could still yield a positive test result overall. We change the logic to t.Error only if the timeout hits (that is, we fail to collect the desired configuration updates in time).
traefiker
force-pushed
the
dont-t-error-in-file-test-watcher-loop
branch
from
485aeed
to
b5966e0
Compare
This was referenced Sep 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Change the logic in
file_test.go'sTestProvideWithWatchverification loop tot.Error/t.Failonly if the timeout hits (that is, we fail to collect the desired configuration updates in time).Motivation
The loop in
TestProvideWithWatchis meant to be executed multiple times until either all configuration elements are found or the timeout expires. By callingassert.*(such asassert.Len), however, an assertion failure in a non-terminal loop run will causet.Errorto be invoked, thereby marking the whole test as failed even though subsequent loop runs could still yield a positive test result overall.Additional Notes
This test has failed repeatedly (albeit not consistently) in our (HolidayCheck's) local CI. I couldn't exactly pinpoint the reason, but I suspect different concurrency behavior and/or CI machine specs. Still, the logic as it is implemented now seems incorrect for the reasons I outlined above (
t.Errorshould only be called on terminal test errors), and thus the implementation change should be an improvement IMHO.