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

Fixed flappers and test_SSLHandshakeFirst by skipping if server is < 2.10.0 #784

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Aug 7, 2024

Signed-off-by: Ivan Kozlovic [email protected]

@kozlovic kozlovic requested a review from levb August 7, 2024 22:30
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.85%. Comparing base (1553d4a) to head (7887208).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #784      +/-   ##
==========================================
+ Coverage   68.71%   68.85%   +0.14%     
==========================================
  Files          39       49      +10     
  Lines       15207    15219      +12     
  Branches     3143     3133      -10     
==========================================
+ Hits        10449    10479      +30     
+ Misses       1700     1692       -8     
+ Partials     3058     3048      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Added missing `-a 127.0.0.1` in some tests to prevent the server
from sending other IP addresses to the client.

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic kozlovic changed the title Fixed SSLHandshakeFirst test by skipping if server is < 2.10.0 Fixed flappers and test_SSLHandshakeFirst by skipping if server is < 2.10.0 Aug 8, 2024
@@ -18352,7 +18352,7 @@ void test_GetClientID(void)
testCond(true);
return;
}
pid1 = _startServer("nats://127.0.0.1:4222", "-cluster nats://127.0.0.1:6222 -cluster_name abc", true);
pid1 = _startServer("nats://127.0.0.1:4222", "-a 127.0.0.1 -p 4222 -cluster nats://127.0.0.1:6222 -cluster_name abc", true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kozlovic is this (extra server flags) a windows-specific fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. When we just specify the port, the server uses 0.0.0.0 for the TCP listen address, which then can resolve to multiple IPs that are sent to the client through gossip protocol. What I have seen (in my Windows VM, and probably what is happening on Windows CI) is that the server would return multiple addresses (say 192.168.x.y but also 10.5.x.y, etc..), which "interfere" with the tests expectations. On Linux or other CIs that we are running it on, there was probably nothing returned (maybe it is 127.0.0.1).
So the "fix" is not really Windows specific. It is simply making the test more deterministic.

Copy link
Collaborator

@levb levb left a comment

Choose a reason for hiding this comment

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

LGTM

@levb levb merged commit 01cfb28 into main Aug 8, 2024
27 checks passed
@levb levb deleted the fix_test branch August 8, 2024 13:30
github-actions bot pushed a commit that referenced this pull request Aug 8, 2024
github-actions bot pushed a commit to levb/nats.c that referenced this pull request Aug 8, 2024
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.

2 participants