test: Remove intermittently failing and not very meaningful BOOST_CHECK in cnetaddr_basic #21689

1. 
   practicalswift commented at 6:28 am on April 15, 2021: contributor

   Remove intermittently failing and not very meaningful BOOST_CHECK in cnetaddr_basic.

   Fixes #21682.

   Rationale from #21682 (comment):

   I’ve looked at that test before and I don’t think that specific BOOST_CHECK makes much sense TBH :)

   1.) 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?

   2.) And perhaps more fundamentally: why would we even want to have %zone_index in our textual ToString() output? I think the expectation is to get say fe80::1ff:fe23:4567:890a (without zone index) and not say fe80::1ff:fe23:4567:890a%eth2 or fe80::1ff:fe23:4567:890a%3 when doing ipv6_addr.ToString() :)

2. test: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic` 63631beef6
3. fanquake added the label Tests on Apr 15, 2021
4. MarcoFalke added this to the milestone 22.0 on Apr 15, 2021
5. 
   DrahtBot commented at 7:37 pm on April 15, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21690 (test: use higher value and per-platform assert in cnetaddr link-local test by jonatack)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

6. 
   jonatack commented at 7:54 pm on April 15, 2021: member

   @practicalswift Thank you for your concerns that have been helpful. I’ve updated #21690 to fix the new issue and make the scoped addr assertion be per-platform (macOS, and all others). I hope it addresses your feedback to make the coverage more meaningful.
7. 
   jonatack commented at 8:02 pm on April 15, 2021: member

   (Whether it’s better now or not, thanks to your feedback, you made me improve it more than I would have done otherwise and that is a good thing.)
8. 
   MarcoFalke commented at 9:18 am on April 17, 2021: member

   review ACK 63631beef6a0046390469971adf4500718ab34ad

   I don’t have any background in ip addresses, but it seems fine to remove a recently added test that intermittently fails. Looks like #21690 reworks the test. That pull can land after rebase+review.

9. MarcoFalke merged this on Apr 17, 2021
10. MarcoFalke closed this on Apr 17, 2021

    ---

11. 
    jonatack commented at 9:30 am on April 17, 2021: member

    Note that the test was added seven months ago and no issues reported until this week.

    Re-working the test was not needed to fix this issue. It was only done in reponse to extraneous, new feedback unrelated to the issue.

12. 
    MarcoFalke commented at 9:50 am on April 17, 2021: member

    I am seeing intermittent issues for years that are still waiting for someone to report an issue on. So a lack of a report doesn’t imply the issue didn’t exist when it was introduced.
13. 
    jonatack commented at 10:48 am on April 17, 2021: member

    Sure, but the fix I proposed was a smaller change than this one (two lines changed, one of which was a comment) and did not drop coverage and context. I tried to improve things constructively. Well, I tried.
14. sidhujag referenced this in commit 94b99db6d3 on Apr 17, 2021
15. in src/test/net_tests.cpp:312 in 63631beef6

    306@@ -307,9 +307,6 @@ BOOST_AUTO_TEST_CASE(cnetaddr_basic)
    307     BOOST_REQUIRE(addr.IsValid());
    308     BOOST_REQUIRE(addr.IsIPv6());
    309     BOOST_CHECK(!addr.IsBindAny());
    310-    const std::string addr_str{addr.ToString()};
    311-    BOOST_CHECK(addr_str == scoped_addr || addr_str == "fe80:0:0:0:0:0:0:1");
    312-    // The fallback case "fe80:0:0:0:0:0:0:1" is needed for macOS 10.14/10.15 and (probably) later.
    

    ---

    ---

    

    jonatack commented at 11:30 am on May 17, 2021:

    This change orphaned the documentation in line 303. Fixed in #21961.
16. PastaPastaPasta referenced this in commit 437ecceadc on Jun 27, 2021
17. PastaPastaPasta referenced this in commit 7d6d33b123 on Jun 28, 2021
18. PastaPastaPasta referenced this in commit 45711c5738 on Jun 29, 2021
19. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift DrahtBot jonatack MarcoFalke

Labels
Tests

Milestone
22.0