Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure all updates are processed in xds ir update test #1799

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

dboslee
Copy link
Contributor

@dboslee dboslee commented Aug 18, 2023

What this PR does / why we need it:
TestXdsIRUpdates added in #1795 is flaky. This adds a sleep to reduce the chance of failing. Open to suggestions to avoid the sleep but this pattern is also used in another test here

time.Sleep(100 * time.Millisecond)

@dboslee dboslee requested a review from a team as a code owner August 18, 2023 22:25
Signed-off-by: David Boslee <david@goteleport.com>
@arkodg
Copy link
Contributor

arkodg commented Aug 18, 2023

how about using require.Eventually e.g. in here ?

@arkodg
Copy link
Contributor

arkodg commented Aug 18, 2023

would be good to test this with go test ./... -count=20

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #1799 (50024ee) into main (8398049) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1799      +/-   ##
==========================================
+ Coverage   65.04%   65.12%   +0.08%     
==========================================
  Files          84       84              
  Lines       12230    12230              
==========================================
+ Hits         7955     7965      +10     
+ Misses       3764     3756       -8     
+ Partials      511      509       -2     

see 2 files with indirect coverage changes

Signed-off-by: David Boslee <david@goteleport.com>
@dboslee
Copy link
Contributor Author

dboslee commented Aug 19, 2023

how about using require.Eventually e.g. in here ?

@arkodg updated in 5fcae5d

require.Eventually didn't exactly fit here. Checking that the number of updates is eventually equal to the desired value could work but could also give a false positive where the number of updates would have been incremented past the desired value otherwise.

This updated version of the test uses the start and end keys to indicate the first and last updates so we know when to close the map and ensure that all events have been processed.

I ran go test -timeout 10s -run ^TestXdsIRUpdates$ github.com/envoyproxy/gateway/internal/message -count 1000 successfully.

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks !

@dboslee dboslee changed the title fix: add sleep to reduce chance of test failure fix: ensure all updates are processed in xds ir update test Aug 20, 2023
@zirain zirain merged commit 23652af into envoyproxy:main Aug 21, 2023
18 checks passed
@dboslee dboslee deleted the david/fix-flaky-test branch August 21, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants