Add routing stategy conflict integration test #699
Merged
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
==========================================
+ Coverage 80.20% 83.01% +2.80%
==========================================
Files 64 76 +12
Lines 4492 5793 +1301
==========================================
+ Hits 3603 4809 +1206
- Misses 600 649 +49
- Partials 289 335 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mikenairn
approved these changes
Jun 12, 2024
There was a problem hiding this comment.
Test seems fine. Some thoughts, but don't have to block:
- It might be good to document the test a bit more, it's on the more complex side and won't be immediately obvious looking at it, especially in a few weeks, what it is testing.
- I'd avoid using https listeners unless you are testing https (Doens't really make any difference since we are mocking the gateway status anyway, just looks like it's important to the test when i see it)
- I'd avoid using
multicluster.MultiClusterIPAddressTypeunless you are writing a test to explicitly test that. Most of our tests now should just use the normal gateway status since that is what we now use in all deployment cases (distributed dns) with the OCM integration being lower priority for now.
| })), | ||
| ) | ||
| // long timeout in a separate assertion - this avoids the test from being flaky: sometimes policy needs more time to become enforced | ||
| }, TestTimeoutLong, TestRetryIntervalMedium).Should(Succeed()) |
There was a problem hiding this comment.
How long do we expect this to take? Does Enforced being true only get set once the record is ready?
There was a problem hiding this comment.
Yes, Enforced can only be set once it is Ready. On my local runs from 2-3 to 12 seconds (that section). The test itself usually around a minute
There was a problem hiding this comment.
Updated test:
- added more comments
- using the HTTP listener
- using casual IP address
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.
Adds a new integration test to ensure we deal correctly with the situation when two policies will have conflicting strategies for the same host