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

support for system-assigned ports during test #346

Merged
merged 8 commits into from Aug 25, 2020

Conversation

apenn-msft
Copy link
Contributor

Allow ares-test to use a system-assigned port (by passing port value 0) which will use the next available port assigned by the system.
Context: In a unit test sandbox environment, it is difficult to control what ports will be available at the time of running the test, and a default or manually-selected port can fail to bind if it is in use. The binding to a system-assigned port guarantees the port is available.

@coveralls
Copy link

coveralls commented Aug 24, 2020

Coverage Status

Coverage increased (+0.009%) to 89.154% when pulling 60cbcbd on apenn-msft:system-ports into 2c32de2 on c-ares:master.

@apenn-msft apenn-msft closed this Aug 24, 2020
@apenn-msft apenn-msft reopened this Aug 24, 2020
@bradh352
Copy link
Member

Looks like the travis builds are all failing. A quick glance says its due to int vs socklen_t. You should probably use ares_socklen_t instead.

Also, instead of EXPECT_NE(SOCKET_ERROR, result); use EXPECT_EQ(0, result); ... its more portable.

@apenn-msft
Copy link
Contributor Author

Looks like the travis builds are all failing. A quick glance says its due to int vs socklen_t. You should probably use ares_socklen_t instead.

Also, instead of EXPECT_NE(SOCKET_ERROR, result); use EXPECT_EQ(0, result); ... its more portable.

on it, thanks!

comparing SOCKET_ERROR based on the only existing example I could find of getsockname in ares_sortaddinfo.c
if (getsockname(sock, src_addr, &len) == -1)

@bradh352
Copy link
Member

Just trying to evaluate your changeset a little further. I'm not actually seeing where the port could be specified as zero to trigger your new logic except if you pass -p 0 to the test harness maybe. Is that how you intend on this to be tested?

What about when MockChannelOptsTest() passes a value > 1 for count. I see a few tests that pass a value of '3' for count, so that would pass ports 0, 1, 2 in those tests if you used -p 0.

@apenn-msft
Copy link
Contributor Author

Just trying to evaluate your changeset a little further. I'm not actually seeing where the port could be specified as zero to trigger your new logic except if you pass -p 0 to the test harness maybe. Is that how you intend on this to be tested?

Yep, that's the intention.

What about when MockChannelOptsTest() passes a value > 1 for count. I see a few tests that pass a value of '3' for count, so that would pass ports 0, 1, 2 in those tests if you used -p 0.

good point. let me look into that

@bradh352
Copy link
Member

It might be better to go ahead and see what happens if you default to 0 too. There's probably no reason to use a specific port with your change (but allow the -p to override it). And just handle that loop case differently if the mock_port is 0, to always pass 0 instead of incrementing so it gets a system-assigned port.

@apenn-msft
Copy link
Contributor Author

I've fixed the multi-server base port scenario; I'll try changing the default to 0 now. I'm actually curious if the 0-mean-system-assigned port is a universally agreed-upon (i.e. POSIX?) concept or if there is a define I should be using for it.

test/ares-test.cc Show resolved Hide resolved
… the default value is ambiguous with the system-assigned port
@bradh352
Copy link
Member

Yes, 0 is portable for a system-assigned port to my knowledge.

@bradh352 bradh352 merged commit 5b246d2 into c-ares:master Aug 25, 2020
3 checks passed
@apenn-msft apenn-msft deleted the system-ports branch August 26, 2020 00:25
sergepetrenko pushed a commit to tarantool/c-ares that referenced this pull request Jul 29, 2022
c-ares#346)

The c-ares test suite was hardcoded to use port 5300 (and possibly 5301, 5302) for the test suite.  Especially in containers, there may be no guarantee these ports are available and cause tests to fail when they could otherwise succeed.  Instead, request the system to assign a port to use dynamically.  This is now the default.  To override, the test suite still takes the "-p <port>" option as it always has and will honor that.

Fix By: Anthony Penniston (@apenn-msft)
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