Test and fix more combinations for PosixSocketResolve #4827
Conversation
jellefoks
force-pushed
the
resolver_tests_and_localhost
branch
from
3b107c9 to
c34a2f7
Compare
jellefoks
force-pushed
the
resolver_tests_and_localhost
branch
7 times, most recently
from
ac49e67 to
650904f
Compare
jellefoks
changed the title
jellefoks
force-pushed
the
resolver_tests_and_localhost
branch
2 times, most recently
from
83c6418 to
7e88efa
Compare
jellefoks
requested review from
maxz-lab,
kaidokert,
johnxwork and
hlwarriner
|
Removed on-device label. The only remaining failures were issues not related to this PR. |
This adds more combinations of parameters for LocalHost and SunnyDay PosixSocketResolveTest tests, including insuring that lookups work as expected with address family, socket type, and protocol hints. This will ensure that PosixSocketResolveTest will also detect systems where IPv6 localhost lookups don't work. b/369361511
Make SunnyDayFlags more forgiving, as well as the error return value for Starboard < 16 and be a little more forgiving for hint flags returned with the results.
jellefoks
force-pushed
the
resolver_tests_and_localhost
branch
from
7e88efa to
533f628
Compare
| ASSERT_NE(nullptr, ai); | ||
| } else { | ||
| #if defined(EAI_ADDRFAMILY) | ||
| EXPECT_TRUE(result == EAI_NONAME || result == EAI_NODATA || |
There was a problem hiding this comment.
I'm wondering why we need all of these or conditions here. Isgetaddrinfo, under the same circumstances under test, permitted to return any four of these values? Or is the EXPECT_TRUE written this way just to handle different parameter values provided to the parameterized test cases?
If the former, I think a comment explaining why any of these four return values can be expected would help.
If the latter, there should probably be some branching on the param values so that the EXPECT_TRUE can be more specific for each.
There was a problem hiding this comment.
These are unfortunately needed at this time because the name lookup behaves slightly differently both in different network conditions as well as on different platforms:
There are different reasons why lookups with a specified address family can fail (e.g. OS doesn't support the address family, or the DNS lookup returns no matches for the address family, etc), and additionally the return value EAI_ADDRFAMILY is not defined on all platforms, and on our win32 builds, for some reason EAI_NONAME == EAI_NODATA, to the platform specific constant WSANO_DATA was needed to check for that return value.
In a world where all builds are hermetic, and all implementations return the correct return values or are wrapped to do so, these conditions can be simplified. Until then, these are needed to pass the test on all tested platforms.
So yes, this is not ideal but before this PR that deficiency of the platform abstraction wasn't even visible because the tests were not exhaustive as they are now.
There was a problem hiding this comment.
Sounds good, thanks for explaining.
| EXPECT_TRUE(result == EAI_NONAME || result == EAI_NODATA || | ||
| result == EAI_FAMILY) | ||
| << " result = " << result; | ||
| #endif |
There was a problem hiding this comment.
nit: it would help with readability to add #endif comments.
| for (const struct addrinfo* i = ai; i != nullptr; i = i->ai_next) { | ||
| EXPECT_EQ(i->ai_addr->sa_family, AF_INET6); | ||
| if (GetAddressFamily() != AF_UNSPEC) { |
There was a problem hiding this comment.
This test has a lot of conditional statements resulting from the heavy parameterization. Given the time sensitivity I definitely wouldn't ask you to make any changes in this PR, but I think it's worth considering whether this test could be broken up into multiple tests and we could sacrifice some code reuse for improved readability.
There was a problem hiding this comment.
Honestly, not parametrizing this and making a copy of the entire test for each parameter set (7 variants), and then each version having only a single EXPECT() slightly different from the others would make the test less readable to me, because it is no longer obvious that the tests are otherwise exactly the same, especially with the platform specific handling that is currently necessary above.
|
Note: |
This adds more combinations of parameters for LocalHost and SunnyDay PosixSocketResolveTest tests, including insuring that lookups work as expected with address family, socket type, and protocol hints.
This will ensure that PosixSocketResolveTest will also detect systems where IPv6 localhost lookups don't work.
This also includes various fixes for IPv6 resolve lookups, fixing the related crashes when used with Starboard 16 in environments with IPv6.
b/369361511
b/377537373