test: use assert helpers in interface_http.py #36396

pull Ayoazeez26 wants to merge 2 commits into bitcoin:master from Ayoazeez26:interface-http-assert-helpers changing 1 files +3 −2

Ayoazeez26 commented at 11:11 PM on September 30, 2026: none

In test/functional/interface_http.py, I replaced assert with assert_greater_than() to make debugging easier on failure by printing both values being compared, instead of the previously set custom message.

Asserts that have their custom message explaining the throttling failure being tested were left unchanged, as the helpers do not accept a message.

Suggested in review of #36324: #36324 (review)

I ran build/test/functional/interface_http.py before and after making the changes and it passed both times

test: use assert helpers in interface_http.py

Replace asserts with assert_greater_than() and
assert_greater_than_or_equal(). This makes debugging easier
on failure by printing both values being compared, instead
of the previously set custom message.

Asserts that have their custom message explaining the
throttling failure being tested are left unchanged,
as the helpers do not accept a message.

Suggested in review of #36324:
https://github.com/bitcoin/bitcoin/pull/36324#discussion_r4102111525

62819acb02

DrahtBot added the label Tests on Sep 30, 2026
DrahtBot commented at 11:11 PM on September 30, 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/36396.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

Concept NACK davidgumberg, l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
davidgumberg commented at 1:49 AM on October 1, 2026: contributor

~crACK 62819acb020ee9d52e6b7697910c2e617c096018~

Edit: I ACK'ed because the code change is correct, but I didn't realize there was conceptual disagreement about this.

Seeing that there is disagreement, and both ways of doing this seem good to me, I'm in favor of leaving the test as-is instead of discussing it, (polite) NACK

Type	Reviewers
Concept NACK	davidgumberg, l0rinc

in test/functional/interface_http.py:103 in 62819acb02

  98 | @@ -97,8 +99,8 @@ def expect_timeout(self, seconds):
  99 |          # not immediately, and not too far over the configured duration.
 100 |          # This allows for some jitter in the test between client and server.
 101 |          duration = stop - start
 102 | -        assert duration <= seconds + 2, f"Server disconnected too slow: {duration} > {seconds}"
 103 | -        assert duration >= seconds - 1, f"Server disconnected too fast: {duration} < {seconds}"
 104 | +        assert_greater_than_or_equal(seconds + 2, duration)
 105 | +        assert_greater_than_or_equal(duration, seconds - 1)

pinheadmz commented at 1:56 AM on October 1, 2026:

What is the benefit of removing the debug information?

Ayoazeez26 commented at 5:42 AM on October 1, 2026:

I see now that the previous output already prints both values and this one removes the descriptive text and just prints both values. Reverting changes here, thanks.

l0rinc commented at 5:18 AM on October 1, 2026: contributor

I'm not a fan of the assert_greater_than_or_equal abominations used to replace <=, so this directionality is a NACK from me...

test: use assert helper in interface_http.py

Replaces assert with assert_greater_than(). It
makes debugging easier on failure by printing both
values being compared.

Asserts that have their custom message explaining the
throttling failure being tested are left unchanged,
as the helpers do not accept a message.

Suggested in review of #36324:
https://github.com/bitcoin/bitcoin/pull/36324#discussion_r4102111525

d2c2ecd32b

in test/functional/interface_http.py:816 in d2c2ecd32b
```
 812 | @@ -812,7 +813,7 @@ def check_slow_read_throttle(self):
 813 |          # Request the big block JSON once to check its size and establish the connection
 814 |          URI = f"/rest/block/{big_block_hash}.json"
 815 |          response_body_size = len(conn.get(URI).read())
 816 | -        assert response_body_size > 7 * 1024 * 1024, f"Big block JSON response size is {response_body_size} bytes"
 817 | +        assert_greater_than(response_body_size, 7 * 1024 * 1024)
```
winterrdog commented at 9:02 AM on October 1, 2026:

I'd lean toward keeping the original assert. The explicit message gives useful context in test logs that assert_greater_than seems to drop.

But anyway, I'd like to know why you want to change this.

Ayoazeez26 commented at 11:40 AM on October 1, 2026:

Thanks for the review, it was first suggested here and why I made the change is that the custom message only states the measured value, while assert_greater_than prints the value and what it was compared against, and the traceback points to this line where the comment explains what is being checked
maflcko commented at 11:30 AM on October 1, 2026: member

Closing for now, due to controversy. In any case, there are several open pulls touching this file, so one of them could trivially cherry-pick this commit.
maflcko closed this on Oct 1, 2026

Contributors

Labels

Linked (view graph)

#36324 test: Fixup MAX_BODY_SIZE http throttling test

test: use assert helpers in interface_http.py #36396

Code Coverage & Benchmarks

Reviews