The intermittently failing BOOST_CHECK is:

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? :)

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.

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

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)? :)

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? :)

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

-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0: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"

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

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

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

BOOST_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:

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

@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.

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?

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.

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.

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

Conflicts

No conflicts as of last run.

Needs rebase after #21689 (comment)

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.

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

@practicalswift does this now address your feedback?

@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.

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.

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

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