qa: Fix Windows logging bug #34282

hebasto commented at 1:29 PM on January 14, 2026: member

The regex (.*) was capturing \r from subprocess output on Windows, causing the closing parenthesis in logs to wrap to the next line.

For example:

208/454 - feature_bip68_sequence.py passed, Duration: 10 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system
)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system
)
211/454 - rpc_packages.py passed, Duration: 8 s
212/454 - rpc_bind.py --nonloopback skipped (not on a Linux system
)
213/454 - p2p_feefilter.py passed, Duration: 4 s

Stripping whitespace from the regex match fixes the formatting. See:

208/454 - feature_bip68_sequence.py passed, Duration: 9 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system)
211/454 - rpc_bind.py --nonloopback skipped (not on a Linux system)
212/454 - rpc_packages.py passed, Duration: 7 s

qa: Fix Windows logging bug

The regex `(.*)` was capturing `\r` from subprocess output on Windows,
causing the closing parenthesis in logs to wrap to the next line.

Stripping whitespace from the regex match fixes the formatting.

979d41bfab

hebasto added the label Questions and Help on Jan 14, 2026

DrahtBot commented at 1:30 PM on January 14, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34282.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

fanquake removed the label Questions and Help on Jan 14, 2026

DrahtBot added the label Tests on Jan 14, 2026

in test/functional/test_runner.py:812 in 979d41bfab

 808 | @@ -809,7 +809,7 @@ def proc_wait(task):
 809 |                      status = "Passed"
 810 |                  elif proc.returncode == TEST_EXIT_SKIPPED:
 811 |                      status = "Skipped"
 812 | -                    skip_reason = re.search(r"Test Skipped: (.*)", stdout).group(1)
 813 | +                    skip_reason = re.search(r"Test Skipped: (.*)", stdout).group(1).strip()

l0rinc commented at 1:51 PM on January 14, 2026:

are we deliberately greedy here?

hebasto commented at 2:08 PM on January 14, 2026:

Why?

I guess it was the initial intention to grab everything from the colon until the end of the line.

l0rinc commented at 2:43 PM on January 14, 2026:

Was wondering if (.*?) or (.+) or (.+)$ or (.+)$ would fix this instead, but this should also be fine - except that we likely don't want to match empty, since we're displaying the match later, so:

                    skip_reason = re.search(r"Test Skipped: (.+)", stdout).group(1).rstrip()

l0rinc commented at 3:39 PM on January 14, 2026:

Please resolve the comment

maflcko commented at 2:10 PM on January 14, 2026: member

lgtm ACK 979d41bfab248990d7d520873d17fe52daa8d402

tested locally via:

>>> print(re.search(r"(.*)", "begin\r\nmore text").group(1).encode('ascii'))
b'begin\r'
>>> print(re.search(r"(.*)", "begin\r\nmore text").group(1).strip().encode('ascii'))
b'begin'

l0rinc changes_requested

l0rinc commented at 2:54 PM on January 14, 2026: contributor

Concept ACK

To document this line better I think we should use + to document that we need those values to be non-empty and maybe use rstrip to document that it's the trailing end lines that we care about, not the possible leading spaces.

hebasto commented at 3:02 PM on January 14, 2026: member

... and maybe use rstrip to document that it's the trailing end lines that we care about, not the possible leading spaces.

Is there a specific value in preserving the possible leading spaces in the logs?

l0rinc commented at 3:06 PM on January 14, 2026: contributor

I don't think so, but that's not buggy currently, so a tighter fix would also do it, right? We probably want to see if we're concatenating multiple spaces, stripping it here would mask that. If you strongly disagree, let me know, I'm also fine with the current version.

hebasto commented at 3:34 PM on January 14, 2026: member

I don't think so, but that's not buggy currently, so a tighter fix would also do it, right? We probably want to see if we're concatenating multiple spaces, stripping it here would mask that. If you strongly disagree, let me know, I'm also fine with the current version.

True, the tightest solution would be to strictly remove the single trailing \r. However, I prefer the current approach because (a) it keeps the diff minimal and (b) the resulting code is easier to reason about (due to the symmetry of strip()).

It’s not that I strongly disagree with your suggestion, that the benefits of the current approach outweigh ones of your suggestions. Since you're open to either, I’d prefer to stick with this one.

l0rinc approved

l0rinc commented at 3:39 PM on January 14, 2026: contributor

lightly tested ACK 979d41bfab248990d7d520873d17fe52daa8d402

fanquake merged this on Jan 15, 2026

fanquake closed this on Jan 15, 2026

fanquake referenced this in commit de79f7d01c on Jan 15, 2026

fanquake commented at 3:24 PM on January 15, 2026: member

Backported to 30.x in #34283.

hebasto deleted the branch on Jan 15, 2026

bitcoin deleted a comment on Jan 16, 2026