Improve macOS development experience, improve tests, and address data races#199
Open
maxmoehl wants to merge 4 commits into
Open
Improve macOS development experience, improve tests, and address data races#199maxmoehl wants to merge 4 commits into
maxmoehl wants to merge 4 commits into
Conversation
To allow running the test suite which does not depend on netlink we need to exclude the files which do as the netlink library only works on Linux. The test suite itself only relies on platform independent go code and can be run on any platform. The added build tags only prevent building packages on platforms where it anyway would've failed. Signed-off-by: Maximilian Moehl <maximilian@moehl.eu>
The test suite used differing loopback addresses from the 127.0.0.0/8 range to differentiate clients. Since the client port is already selected at random we do not need to specify a different bind address. Selecting a different bind address breaks the tests on macOS since it only has one loopback address. This commit switches all occurrences where a bind address was passed to be the empty string which lets the OS pick the bind address. The parameter is no longer used by any caller and should be removed, but since it is part of the public API that is a breaking change which will be done separately. Signed-off-by: Maximilian Moehl <maximilian@moehl.eu>
The tests are using time.Sleep liberally which have to be conservative to ensure the condition is met before testing it causing the tests to take longer than needed. This commit reworks the test cases to rely on Eventually and Consistently instead which pass as soon as the condition is met significantly reducing the test runtime. Signed-off-by: Maximilian Moehl <maximilian@moehl.eu>
Running the tests with the race detector enabled flagged multiple cases in which concurrent access was not properly synchronized. This commit addresses those cases. What remains to be done is a comprehensive review of all locks and their usage. It seems like some of them could be simplified and there are some cases in which deadlocks may occur due to multiple locks being held at the same time. Signed-off-by: Maximilian Moehl <maximilian@moehl.eu>
maxmoehl
force-pushed
the
maxmoehl/enhance
branch
from
ec6469e to
f2b2dea
Compare
mkalcok
approved these changes
Jun 30, 2026
mkalcok
left a comment
mkalcok
left a comment
There was a problem hiding this comment.
Overall LGTM, thank you, these are some fine QOL changes. I left some comments in-line, but none of them are blocking, I'll leave it up to you whether to address them or not.
Regarding merging strategy, this PR contains a bunch of logically separate commits (connected by a theme), so I'd suggest rebase rather than squash.
| err := mbClient.AddPeer(serverAddress, "") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
I'll cheer every time there's a hardcoded sleep removed from tests 🎉
| RetryIntervalMax = savedRetryMax | ||
| }() | ||
|
|
||
| totalClients := 50 |
There was a problem hiding this comment.
Why was this changed from 600 to 50? It seems slightly out of scope for this commit. If it's relevant for this commit, it should probably be mentioned in the commit message, a it's a significant change.
| timeout := time.Duration(p.keepaliveInterval) * time.Second * 5 / 2 | ||
| p.log().Infof("KEEPALIVE timeout: %v", timeout) | ||
| p.keepaliveTimer = time.NewTimer(timeout) | ||
|
|
There was a problem hiding this comment.
What was the motivation for pulling this block from keepaliveLoop?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
See individual commits for detailed descriptions.