Skip IPv{4,6} tests when IPv{4,6} loopback doesn't work (#39798) #46481
Conversation
) If the computer on which libstd tests are being run has been configured without support for IPv6 or IPv4, then all of the net::{tcp,udp} tests for that address family will fail. This patch detects this condition and skips all of the affected tests. The automatic detector works by attempting to bind a socket on the appropriate loopback address (::1 or 127.0.0.1). If this succeeds, the tests are run; if it fails with EADDRNOTAVAIL, they are skipped. This is not exactly the same thing as "computer was configured without support for IPvX", but it _is_ the failing operation in most of the problem tests on my computer that doesn't do IPv6. Because the automatic detector might not be perfectly accurate, and because one might _want_ the tests to fail if the computer has been configured incorrectly (this would be appropriate for the CI bots, for instance), it can be overridden with environment variables: set `RUST_TEST_IPV{4,6}=0` to skip the IPv{4,6} tests unconditionally, or `RUST_TEST_IPV{4,6}=1` to run them unconditionally.
kennytm added the S-waiting-on-review 
shepmaster added the T-libs-api 
| It looks like this one slipped automatic assignment. If you'd be so kind as to review this sooner rather than later, randomly assigned... r? @sfackler |
| Thoughts @alexcrichton? I'm not sure how much effort we want to put into workarounds for obscure system configurations. |
| Hm yeah unfortunately we have a pretty bad track record with auto-ignoring tests. For example we have a bunch of valgrind tests in the repository for ensuring that libstd doesn't leak memory, and a few weeks after we auto disabled them if valgrind wasn't installed we stopped running them on the bots (by accident) and then they've never been re-enabled. I would personally prefer to opt-in to skipping tests, if at all. That being said it's very difficult to maintain guarantees like this over time. It's basically guaranteed that new tests will be written that forget to perform these checks (or, for example, run-pass tests exercise functionality outside of libstd). My ideal preference would just be to say "this is what you need to run the test suite" rather than getting the test suite to run in all situations all the time. |
| I don't think lack of working IPv6 (even on the loopback interface) is an "obscure" system configuration, but I can definitely see where automatic skipping could lead to problems with things never getting tested. How would you feel about the patch if I removed the automatic detection but kept the environment variables? Then it's possible to skip them but it only ever happens if specifically requested. Maintaining the set of tests that get suppressed is on people like me who need the knob. |
| It's apparently obscure to some extent at least or this would have come up at some point in the previous 5 or whatever years these tests existed. |
| While it's possible to leave the environment variable in and solve the very immediate problem as well as the longer term problem of "it's harder to forget to run tests", I'm not personally a huge fan of such solutions. They tend to still require a relatively high level of maintenance over time as we'll add new tests that don't check this env var (and then PRs will be made to make the tests check the env var) or we'll be having PRs over time which encourage use of the env var (such as all this). I'm personally more in favor of just documenting the environments that the test suite runs in, and to me at least it seems pretty reasonable to expect ipv4/ipv6 networking to work in a test environment. |
| Given the last comment by @alexcrichton, I think this PR should either be changed to simply improving the documentation, or just be closed (but then currently our CI is hit by travis-ci/travis-ci#8891 🤔). |
kennytm added S-waiting-on-author | I understand where @alexcrichton is coming from, but I think that a configuration with no working IPv6 at all (not even loopback) is not abnormal in the slightest, and deserves to be catered for. |
| Do others from @rust-lang/infra have thoughts on this? I suspect it affects more infra than libs per se.
True! That being said I'd much rather have the tree broken by accident for a day or two then to silently stop running IPv6 tests, accidentally introduce a regression a year later, and then force a point release to get issued because no one noticed. |
kennytm added T-infra 
alexcrichton added S-waiting-on-team 
| The first thing that springs to my mind is that we won't be testing these environment variables ourselves - I don't see us adding additional builders with different configurations just to make sure test selection works properly, and so we are faced with lists of swappable tests that may or may not be out of date. Since our CI setup will not pick up when these environment variables end up selecting the wrong tests, it makes sense to put the lists of tests where we know they can be checked - where the configuration actually exists. This means on the computer actually attempting to run the tests. So I'd see a possible way forward in providing a way for people to explicitly exclude tests themselves - libtest can do this, but it's not passed down anywhere from tools/compiletest. Then you can select a set of tests you don't care about, and be happy with exit code 0 after running the full rust test suite. Just to restate, we're just talking about a situation where some rust compiler tests don't pass on particular configurations - this seems like a fairly strong definition of "catered for". An ignore list so people can select their own set of tests doesn't sound unreasonable. |
kennytm added S-waiting-on-author | Hi @zackw (cc @rust-lang/infra) The infra team had a discussion of this PR last week, the conclusion is basically the same as what @aidanhs described above in #46481 (comment). Could you see if it's easy to repurpose this PR in that direction? Thanks! r? @aidanhs |
| @kennytm First, I need to say my volunteer hacking time is extremely limited from now through August-ish, and I can't promise to do anything in a timely fashion. If you would prefer to close this PR because of that, that's fine with me, I'll cycle back around to it eventually. Or maybe I'll move and get an ISP that plays nicer with IPv6 and this will cease to be an issue for me. :-) (You might think that working external IPv6 would have nothing to do with working loopback IPv6, but it is not so; the only reliable way I have found to prevent my computer from ever issuing IPv6-related DNS queries, which the ISP silently discards, is to disable all IPv6.) Now, to the actual request. I understand you to be asking for a mechanism that enables one to skip tests by name (pattern). That does seem worthwhile to me, but it's an awfully big Twinkie, and it wouldn't actually address the IPv6 issue, because all of the tests that try to use IPv6 do it by looping over the Also, given that the code to support the RUST_DISABLE_IPV[46] environment variables is concentrated in the |
| The intention would be that if your local setup does not match the 'perfect' builder setup for a test, you'd just disable it. So in your case, you'd just disable the network tests completely, since the test expects IPv6.
This quote is interesting because it hints at our position - what happens when another (buggy, I agree) test gets added that doesn't use If instead there is one consistent approach to test exclusion on the user end, they're much more likely to know about it and can just list the ones that failed - away they go, no need to ask here. I'm going to take a look at write up mentoring instructions for test-exclude-passthrough-to-libtest since it seems like a useful thing to fix so a newcomer can get stuck in. |
Thank you very much for your contribution, @zackw ! I'm going to go ahead and close this PR for now for the reasons you mentioned. Please feel free to re-open it if you find yourself with some extra free time! |
shepmaster added S-inactive 6 participants
If the computer on which libstd tests are being run has been configured without support for IPv6 or IPv4, then all of the net::tcp and net::udp tests for that address family will fail. This patch detects this condition and skips all of the affected tests.
The automatic detector works by attempting to bind a socket on the appropriate loopback address (::1 or 127.0.0.1). If this succeeds, the tests are run; if it fails with EADDRNOTAVAIL, they are skipped. This is not exactly the same thing as "computer was configured without support for IPvX", but it is the failing operation in most of the problem tests on my computer that doesn't do IPv6.
Because the automatic detector might not be perfectly accurate, and because one might want the tests to fail if the computer has been configured incorrectly (this would be appropriate for the CI bots, for instance), it can be overridden with environment variables: set
RUST_TEST_IPV{4,6}=0to skip the IPv{4,6} tests unconditionally, orRUST_TEST_IPV{4,6}=1to run them unconditionally.While I was at it I tightened up some inefficient code in libstd/net/test.rs a bit:
base_portno longer callsgetcwdevery time it's used.Every test that directly calls
test_ipv4_pshould probably be adjusted to useeach_ipbut that would be a whole project in itself.