test: Remove intermittently failing and not very meaningful BOOST_CHECK in cnetaddr_basic#21689
Conversation
…ECK` in `cnetaddr_basic`
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
@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. |
|
(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.) |
|
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. |
|
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. |
|
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. |
… meaningful `BOOST_CHECK` in `cnetaddr_basic` 63631be test: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic` (practicalswift) Pull request description: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic`. Fixes bitcoin#21682. Rationale from bitcoin#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()` :) ACKs for top commit: MarcoFalke: review ACK 63631be Tree-SHA512: 06863d1edfb9ad1ca9bcae09cf3f0f47b58bb29d222b70799c3dc059b96452889026e4b99b132782846d9896e3e798d17c7f9406e0e6a0bec1bffc6edb54e9df
| 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. |
There was a problem hiding this comment.
This change orphaned the documentation in line 303. Fixed in #21961.
5 participants
Remove intermittently failing and not very meaningful
BOOST_CHECKincnetaddr_basic.Fixes #21682.
Rationale from #21682 (comment):