-
Notifications
You must be signed in to change notification settings - Fork 25
Integration tests keep connections in batches #2101
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
Integration tests keep connections in batches #2101
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #2101 +/- ##
=======================================
Coverage 28.52% 28.52%
=======================================
Files 94 94
Lines 5757 5757
Branches 2547 2547
=======================================
Hits 1642 1642
Misses 3393 3393
Partials 722 722
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
2263802
to
6bfe170
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JoukoVirtanen - I've reviewed your changes - here's some feedback:
- Consider renaming
Connections
toConnectionsByScrape
andGetAllConnections
toConnections
for clarity, although this might require more refactoring.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just some minor nitpicks.
3259cc0
to
a7adacc
Compare
@@ -150,26 +149,48 @@ func (m *MockSensor) LiveConnections() <-chan *sensorAPI.NetworkConnection { | |||
|
|||
// Connections returns a list of all connections that have been received for | |||
// a given container ID | |||
func (m *MockSensor) Connections(containerID string) []types.NetworkInfo { | |||
func (m *MockSensor) Connections(containerID string) []types.NetworkInfoBatch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the Connections
function operating in the way it always has and then add a new function to retrieve the batches. This will mean that existing tests that do not care about batch consistency can remain unchanged, while new tests that verify batch consistency can use the new API to perform their tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@JoukoVirtanen, rebasing should fix the arm integration test failures you are seeing. |
…the expected scrapes
a7adacc
to
622cd92
Compare
Description
Modified integration tests such that instead of simply maintaining a list of connections seen for each container, information about which scrape the connections were seen in is also maintained. To be more precise instead of having a map where the key is the container ID and the value is a list of all connections seen, there is now a map where the key is the container ID and the value is a 2D slice where the first index is the scrape. Scrapes with no connections are not saved, so there are no empty slices of connections.
This change was made to better test changes to the connections sent when runtime configuration is changed. More specifically if external IPs is initially disabled and then enabled, normalized connections should be reported closed in the same scrape interval as the unnormalized connections are reported as being open. Similar logic applies to when external IPs is initially enabled and then disabled.
That work was done here #2068
The integration tests here will continue to fail until the above PR is merged.
Checklist
Automated testing
- [ ] Added unit tests- [ ] Added integration tests- [ ] Added regression testsThis just changes some integration tests so these things are not needed.
Testing Performed
These integration tests were run without the changes from #2068 and failed as expected and are now passing with those changes, showing that these changes work and are useful.