test: use higher value and per-platform assert in cnetaddr link-local test

jonatack commented at 7:21 am on April 15, 2021: member

This patch improves the cnetaddr link-local test in net_tests by making it platform-specific and by using a higher scoped id value to alleviate recent CI issues.

Closes #21682.

fanquake added the label Tests on Apr 15, 2021

practicalswift commented at 7:35 am on April 15, 2021: contributor

The intermittently failing BOOST_CHECK is:

https://github.com/bitcoin/bitcoin/blob/9712f75746e3da73471da2e23a4bfc1382c69308/src/test/net_tests.cpp#L310-L312

I don’t understand why we test if ToString() output includes %zone_index: it clearly doesn’t on some platforms, so we cannot rely on it anyways. Then why test it? :)

jonatack commented at 7:39 am on April 15, 2021: member

My understanding when working on this was that Apple might be going in its own direction with this starting with macOS 10.14 but it still seemed worthwhile to test our code on platforms like Linux and Windows where we can.

jonatack commented at 7:42 am on April 15, 2021: member

(Without the fallback, only the macOS 10.14 CI was failing. So coverage on the other CIs seemed good to have.)

practicalswift commented at 7:49 am on April 15, 2021: contributor

My understanding when working on this was that Apple might be going in its own direction with this starting with macOS 10.14 but it still seemed worthwhile to test our code on platforms like Linux and Windows where we can.

But the test is not conditional on platform, so AFAICT we’re testing that ToString either contains %zone_id (the addr_str == scoped_addr case) or that it doesn’t contain %zone_id (the addr_str == "fe80:0:0:0:0:0:0:1" case)? :)

https://github.com/bitcoin/bitcoin/blob/9712f75746e3da73471da2e23a4bfc1382c69308/src/test/net_tests.cpp#L310-L312

In other words: we’re effectively not testing anything meaningful in a test that is intermittently failing, no? :)

If so, why not simply remove it? :)

jonatack commented at 7:57 am on April 15, 2021: member

If you test only the fallback, I found that the change fails on all CIs except the macOS 10.14 one:

0-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
1+    BOOST_CHECK(addr_str == "fe80:0:0:0:0:0:0:1");

$ src/test/test_bitcoin -t net_tests Running 14 test cases… test/net_tests.cpp(311): error: in “net_tests/cnetaddr_basic”: check addr_str == “fe80:0:0:0:0:0:0:1” has failed

*** 1 failure is detected in the test module “Bitcoin Core Test Suite”

MarcoFalke commented at 8:03 am on April 15, 2021: member

Does this actually fix the issue or make it only less likely?

jonatack commented at 8:59 am on April 15, 2021: member

As far as I know, the issue began appearing in the CI only this week, so it may be from some recent change in the CI setup, as the tested value of 32 wasn’t very high. The CI is green here, so re-pushing now with a very high value (4096). If that causes a failure down the line, I guess we could drop the coverage as suggested above.

jonatack force-pushed on Apr 15, 2021

practicalswift commented at 10:54 am on April 15, 2021: contributor

@jonatack

I didn’t get if you agree with my statement that the intermittently failing BOOST_CHECK is basically doing:

0BOOST_CHECK(to_string_does_contain_zone_id || to_string_does_not_contain_zone_id);

And if so, is there any point in keeping it around? :)

FWIW, patch against current master to show the issue more clearly:

 0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
 1index 8eab26f3d..e7c3475ef 100644
 2--- a/src/test/net_tests.cpp
 3+++ b/src/test/net_tests.cpp
 4@@ -308,8 +308,9 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
 5     BOOST_REQUIRE(addr.IsIPv6());
 6     BOOST_CHECK(!addr.IsBindAny());
 7     const std::string addr_str{addr.ToString()};
 8-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
 9-    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15 and (probably) later.
10+    const bool to_string_does_contain_zone_id = addr_str == scoped_addr;
11+    const bool to_string_does_not_contain_zone_id = addr_str == "fe80:0:0:0:0:0:0:1";
12+    BOOST_CHECK(to_string_does_contain_zone_id || to_string_does_not_contain_zone_id);
13     // Test that the delimiter "%" and default zone id of 0 can be omitted for the default scope.
14     BOOST_REQUIRE(LookupHost(link_local + "%0", addr, false));
15     BOOST_REQUIRE(addr.IsValid());

jonatack commented at 12:51 pm on April 15, 2021: member

@practicalswift It looks like I should document the test better to communicate the context and information. I’ll propose that separately. Edit: and trying testing by platform to address your concerns.

fanquake commented at 1:57 pm on April 15, 2021: member

It looks like I should document the test better to communicate the context and information. I’ll propose that separately.

Why can’t that be done as part of this change?

jonatack commented at 3:28 pm on April 15, 2021: member

Ok. The CI has been green, and #21696 showed that macOS remains the only CI that fails the scoped_addr case.

Let’s try (a) improving the documentation and (b) testing scoped_addr by platform.

Edit: will update to BOOST_CHECK_EQUAL if this works.

jonatack force-pushed on Apr 15, 2021

jonatack commented at 6:49 pm on April 15, 2021: member

Great–the per-platform scoped address test is green along with the previous two CI runs. Pushing an updated version with BOOST_CHECK( == ) replaced by BOOST_CHECK_EQUAL( , ) for a 4th CI run. I hope this addresses the feedback above while maintaining coverage and adding clarity and context.

jonatack force-pushed on Apr 15, 2021

jonatack renamed this:
~~test: use higher value in cnetaddr_basic link-local test~~
test: use higher value and per-platform assert in cnetaddr link-local test
on Apr 15, 2021

DrahtBot commented at 7:35 pm on April 15, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

MarcoFalke commented at 9:20 am on April 17, 2021: member

Needs rebase after #21689 (comment)

in src/test/net_tests.cpp:305 in 9005fb5005 outdated

299@@ -300,16 +300,21 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
300 
301     // IPv6, scoped/link-local. See https://tools.ietf.org/html/rfc4007
302     // We support non-negative decimal integers (uint32_t) as zone id indices.
303-    // Test with a fairly-high value, e.g. 32, to avoid locally reserved ids.
304+    // Test with a high value, e.g. 4096, to avoid any local ids in use.
305     const std::string link_local{"fe80::1"};
306-    const std::string scoped_addr{link_local + "%32"};
307+    const std::string scoped_addr{link_local + "%4096"};

MarcoFalke commented at 9:44 am on April 17, 2021:

Is there a reference that 4096 is guaranteed to be unused or is this something that depends on the operating system and the programs currently running?

jonatack commented at 2:59 pm on April 17, 2021:

Good question. From what I understand, the scope ids / zone indices depend on the number of local network interfaces when there are multiple on a single link-local address. Whereas many machines may have the first 8 or 16 decimal integer scope ids in use, the previous test value of 32 seems to be on the high end of the range. I’m not aware of one that is guaranteed to be free, but a value in the thousands (or tens of thousands) might be reasonably worth trying.

Resources:

jonatack commented at 9:52 am on April 17, 2021: member

If so, why not simply remove it? :)

I think it is better to improve the test and contextual information it provides and I spent time in a way that I thought was constructive and helpful re-verifying and working to do that. Unfortunately, it’s an order of magnitude more work to refute statements like “not very meaningful” in the #21689 PR title and the comment above, than it is to say them and just erase coverage and (I think) valuable context.

DrahtBot added the label Needs rebase on Apr 17, 2021

test: revert commit 63631bee 6b0e879cf1

test: use higher value in cnetaddr_basic link-local test 680d888d65

test: define scoped cnetaddr test by platform 6ab76c0b9f

jonatack force-pushed on Apr 17, 2021

jonatack commented at 11:07 am on April 17, 2021: member

Rebased, added 6b0e879 for context since this change is not adding a test but hopefully improving it.

DrahtBot removed the label Needs rebase on Apr 17, 2021

jonatack commented at 7:29 pm on April 28, 2021: member

@practicalswift does this now address your feedback?

practicalswift commented at 7:56 pm on April 29, 2021: contributor

@practicalswift does this now address your feedback?

The BOOST_CHECK(to_string_does_contain_zone_id || to_string_does_not_contain_zone_id) issue has been addressed.

I think the issue @MarcoFalke raised about the test outcome being dependent on the user’s local configuration remains unfortunately.

in src/test/net_tests.cpp:317 in 6ab76c0b9f

314+    // Expect macOS to return the address in this format, without zone ids.
315+    BOOST_CHECK_EQUAL(addr_str, "fe80:0:0:0:0:0:0:1");
316+#else
317+    // Expect normal scoped address functionality on the other platforms.
318+    BOOST_CHECK_EQUAL(addr_str, scoped_addr);
319+#endif

MarcoFalke commented at 6:47 am on April 30, 2021:

0    BOOST_CHECK(addr.ToString().starts_with("fe80:0:0:0:0:0:0:1"));

Maybe just check the prefix if the suffix is unstable?

jonatack commented at 10:20 am on May 11, 2021:

The suffix is stable but if the first few scope ids are in use locally then they will be another string and not share the same prefix.

jonatack commented at 10:25 am on May 11, 2021: member

This test and informational context involved some trial and error and time to obtain, and I think it’s a shame to throw out the coverage and information, particularly as it seems to be not well-known info and I tried to provide context about that. However, it seems like I’m swimming upstream to provide this coverage and information.

jonatack closed this on May 11, 2021

laanwj added the label Up for grabs on May 11, 2021

jonatack commented at 11:54 am on May 17, 2021: member

The scoped id/zone indices functionality appears to have been dropped entirely by #21756.

jonatack commented at 1:42 pm on December 21, 2021: member

The Up for grabs label on this pull can be removed following the merge of #21985.

fanquake removed the label Up for grabs on Dec 21, 2021

DrahtBot locked this on Dec 21, 2022

test: use higher value and per-platform assert in cnetaddr link-local test #21690

Conflicts