test: #31212 follow up (spelling, refactor)

hodlinator commented at 11:08 pm on December 5, 2024: contributor

Fixes typo caught by spelling linter (https://github.com/bitcoin/bitcoin/runs/33979284676).
Minor but nice refactoring of combine_logs.py change that was suggested late: #31212 (review)

DrahtBot commented at 11:08 pm on December 5, 2024: 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/31433.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc, maflcko, theStack, BrandonOdiwuor, tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Dec 5, 2024

DrahtBot commented at 11:47 pm on December 5, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34005163301

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Dec 5, 2024

hodlinator force-pushed on Dec 5, 2024

hodlinator commented at 11:56 pm on December 5, 2024: contributor

Ran test/lint/lint-python.py locally before publishing PR, didn’t know one also needed to run test/lint/test_runner/ separately, which lead to CI finding this: https://github.com/bitcoin/bitcoin/pull/31433/checks?check_run_id=34005163301

Added newlines to combine_logs.py in latest push to please CI. Less of a win in brevity, but still nicer on the eyes.

DrahtBot removed the label CI failed on Dec 6, 2024

in test/functional/combine_logs.py:91 in 27c9369db3 outdated

93+            chain = 'regtest' # fallback to regtest
94+        case 1:
95+            chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
96+        case _:
97+            raise RuntimeError('Max one debug.log is supported, found several:\n\t' +
98+                               '\n\t'.join([str(f) for f in debug_logs]))

l0rinc commented at 12:51 pm on December 6, 2024:

nit:

0                               '\n\t'.join(str(f) for f in debug_logs))

or

0                               '\n\t'.join(map(str, debug_logs)))

hodlinator commented at 2:36 pm on December 6, 2024:

Functional style++ :+1: Taken.

in test/functional/combine_logs.py:86 in 27c9369db3 outdated

88-        chain = re.search(r'node0/(.+?)/debug\.log$', path.as_posix()).group(1)  # extract the chain name
89-    else:
90-        chain = 'regtest'  # fallback to regtest (should only happen when none exists)
91+    match len(debug_logs):
92+        case 0:
93+            chain = 'regtest' # fallback to regtest

l0rinc commented at 12:59 pm on December 6, 2024:

nit: I don’t like it either, but the rules claim:

Inline comments should be separated by at least two spaces from the statement

0            chain = 'regtest'  # fallback to regtest

hodlinator commented at 2:37 pm on December 6, 2024:

Had no idea that was intentionally idiomatic Python. Done.

l0rinc commented at 1:00 pm on December 6, 2024: contributor

If you edit again please consider updating the copyright headers to -present. I’m mostly okay with it but would prefer some minor nits addressed if you agree with them

refactor test: Cleaner combine_logs.py logic c9fb38a590

chore: Typo Overriden -> Overridden 41d934c72d

hodlinator force-pushed on Dec 6, 2024

l0rinc commented at 2:40 pm on December 6, 2024: contributor

ACK 41d934c72df6449d2ceb2330d05b959b24350d95

hodlinator commented at 2:42 pm on December 6, 2024: contributor

Updated in response to first review.

Also found an edge-case where having a single node0/debug.log file (meaning the test only ran mainnet) will trigger an exception as the regex will not find a “/” both before and after the match group. Not going to fix within the context of this PR though. Seems like an unlikely case.

maflcko commented at 2:45 pm on December 6, 2024: member

lgtm ACK 41d934c72df6449d2ceb2330d05b959b24350d95

theStack approved

theStack commented at 5:08 pm on December 6, 2024: contributor

Nice, I’ve seen the match/case pattern matching more often recently in other projects but wasn’t aware that our minimum supported Python version already includes it 🎉

ACK 41d934c72df6449d2ceb2330d05b959b24350d95

BrandonOdiwuor approved

BrandonOdiwuor commented at 7:20 pm on December 6, 2024: contributor

Code Review ACK 41d934c72df6449d2ceb2330d05b959b24350d95

in test/functional/combine_logs.py:90 in c9fb38a590 outdated

92+        case 0:
93+            chain = 'regtest'  # fallback to regtest
94+        case 1:
95+            chain = re.search(r'node0/(.+?)/debug\.log$', debug_logs[0].as_posix()).group(1)
96+        case _:
97+            raise RuntimeError('Max one debug.log is supported, found several:\n\t' +

tdb3 commented at 7:55 pm on December 7, 2024:

Looks like functional test code raises Exception and RuntimeError. Didn’t see a preference in the developer-notes or in test style guidelines, so I’m assuming use of either is ok.

hodlinator commented at 9:34 pm on December 8, 2024:

Yeah, it’s a bit fuzzy to me what error type should be used. Both Exception and RuntimeError are unfortunately generic. Manually raising AssertionError from a “non-assertion-ish”-function felt wrong, also because of the cause being something external to the combine_logs.py script itself. Curious if you have any good rationale for such cases.

tdb3 commented at 9:49 pm on December 8, 2024:

I agree that manually raising AssertionError wouldn’t have been the way to go (even though it technically was raised in the previous code containing the assert). ValueError might have worked, but RuntimeError seems fine enough, and seems more appropriate to me than Exception (a bit too generic IMO (decent rationale here)).

tdb3 approved

tdb3 commented at 7:57 pm on December 7, 2024: contributor

ACK 41d934c72df6449d2ceb2330d05b959b24350d95

Nice cleanup. Observed nothing unexpected. Reviewed code changes. Exercised case paths for combine_logs.py:read_logs(). Ran linter (only saw the unrelated txmempool spelling issue).

Induced a test failure and created a signet/debug.log file to exercise the default (>=2) path:

 0$ /home/dev/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_rn9jwu58'
 1Traceback (most recent call last):
 2  File "/home/dev/bitcoin/test/functional/combine_logs.py", line 203, in <module>
 3    main()
 4  File "/home/dev/bitcoin/test/functional/combine_logs.py", line 67, in main
 5    log_events = read_logs(testdir)
 6                 ^^^^^^^^^^^^^^^^^^
 7  File "/home/dev/bitcoin/test/functional/combine_logs.py", line 90, in read_logs
 8    raise RuntimeError('Max one debug.log is supported, found several:\n\t' +
 9RuntimeError: Max one debug.log is supported, found several:
10	/tmp/bitcoin_func_test_rn9jwu58/node0/signet/debug.log
11	/tmp/bitcoin_func_test_rn9jwu58/node0/regtest/debug.log

fanquake merged this on Dec 8, 2024

fanquake closed this on Dec 8, 2024

hodlinator deleted the branch on Dec 8, 2024

test: #31212 follow up (spelling, refactor) #31433

Code Coverage & Benchmarks

Reviews