The test doesn't work currently because the log might be finalized before wait_for_debug_log is started, in which case it will assume the log is empty and fail to detect any line.
Fix this by calling wait_for_debug_log first. Fixes #24060.
Also, remove the "random line number" part of the test, because it doesn't really test anything novel. wait_for_debug_log is inherently racy, so will randomly terminate at the exact point or later. So the randomization is already sufficiently covered.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
Would prefer to have the fix and test removal in separate commits, if not separate PRs.
This is needed for the next commit.
Also, it doesn't really test anything novel. wait_for_debug_log is
inherently racy, so will randomly terminate at the exact point or later.
So the randomization is already sufficiently covered by the existing
test.
Thx, done
Also, remove the "random line number" part of the test, because it doesn't really test anything novel. wait_for_debug_log is inherently racy, so will randomly terminate at the exact point or later. So the randomization is already sufficiently covered.
I'd imagine that the "randomly terminate at the exact point or later" from the first part is a kind of randomness with a distribution that is strongly peaked around some point after this "exact point" (depending on hardware etc.). So whether the first part really covers the kind of randomness that the second part seems to depend on many factors, among them the density of the net of predefined lines provided in the first part.
The "random line number" part contributes a more uniform randomness of lines to stop after, so it doesn't seem completely redundant to me. @jamesob : any thoughts on this?
The test was merged two months ago and never fully worked. There are already too many intermittent CI issues and this test isn't going to make it better. In fact, how is this test supposed to catch any issues if a red CI is rerun anyway? An alternative to this pull would be to remove the test completely and merge it again once it fully works.
I do agree that is aims to provide some nice coverage, which is why I opted to fix it here instead of removing. However, I don't see why the burden should be on me to fix everything in this pull. I think the subtest can be re-introduced in a follow-up if deemed useful.
Yes, the test is "messy" and high maintenance and has a very different approach compared to most other functional tests. On the plus side, it covers a lot of init code that is complicated and hard to test, and I think that it has a much higher potential to find bugs compared to other tests.
As for the second part that is removed in this PR: Does this also have a problem leading to intermittent failures? The CI fail from the linked issue happens in the first part.
No, I removed it because it requires a major rewrite of the test framework and test itself to be implemented. I am not going to do that here, unless someone else does it.
ACK https://github.com/bitcoin/bitcoin/pull/24192/commits/fa7b07571f24b6def6effdd4cc1b96c7507bf959
I think it's a shame to remove the randomized subtest, since that may help us catch init issues that we haven't thought to cover with the pre-specified log fragments, but if it's causing more trouble than it's worth from the standpoint of the maintainers then I think some signal is better than no signal.
I think it's a shame to remove the randomized subtest
Anyone is free to add it back anytime. It is just that I don't have the time for it right now and removing it temporarily is needed to fix the bug.
Code Review ACK fa7b07571f24b6def6effdd4cc1b96c7507bf959
It is just that I don't have the time for it right now and removing it temporarily is needed to fix the bug.
That makes sense, just didn't agree with the rationale in OP that the randomized part added nothing.
The issue for a possible follow-up to solve seems to be to retrieve num_total_logs, which is needed for the randomized subtest but not returned by wait_for_debug_log anymore after using a with statement / context manager.
Milestone
23.0