Page MenuHomePhabricator
Differential D18546

[chronik] Improve coverage of avalanche event ws messages
ClosedPublic
Actions

Authored by Fabien on Aug 29 2025, 12:28.

Details

Reviewers
PiRK
tobias_ruck
Group Reviewers
Restricted Project
Commits
rABC556df980d5df: [chronik] Improve coverage of avalanche event ws messages
Summary

Add a new test that mimics the last part of chronik_ws.py (actually creating some duplicata but with a different intent) while using all the subscription methods at the same time. This makes sure we get the expected messages with all these subscription kinds.

The constants removal in chronik_ws.py are unused and found during the test review while building the new one and has been included here because it does not deserve its own diff and is somewhat related.

Depends on D18542.

Test Plan
./test/functional/test_runner.py chronik_ws chronik_ws_avalanche

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.Aug 29 2025, 12:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 29 2025, 12:28
Fabien requested review of this revision.Aug 29 2025, 12:28
Harbormaster completed remote builds in B34278: Diff 55420.Aug 29 2025, 12:49
PiRK accepted this revision.Aug 29 2025, 13:47
PiRK added a subscriber: PiRK.

Looks good, except for 2 nits

test/functional/chronik_ws_avalanche.py
124 ↗(On Diff #55420)

can be simplified, unless i missing a reason why we want to check exact True identity vs any truthy value

Probably not relevant here, but here are the possible edge cases:

>>> 1 == True
True
>>> 2 == True
False
>>> bool(2)
True
>>> 1 is True
<stdin>:1: SyntaxWarning: "is" with 'int' literal. Did you mean "=="?
False
329 ↗(On Diff #55420)

are the two raise actually reachable, after the above assert False?

This revision is now accepted and ready to land.Aug 29 2025, 13:47
Fabien updated this revision to Diff 55424.Aug 29 2025, 13:59

Remove unecessary raise

Fabien added inline comments.Aug 29 2025, 14:01
test/functional/chronik_ws_avalanche.py
124 ↗(On Diff #55420)

I think it doesn't have any impact on the behavior but makes the code more readable (as it tells the reader that the field is expected to be boolean)

329 ↗(On Diff #55420)

No they're not, I can remove them

Harbormaster completed remote builds in B34280: Diff 55424.Aug 29 2025, 14:45
tobias_ruck requested changes to this revision.Aug 30 2025, 09:10
tobias_ruck added a subscriber: tobias_ruck.
tobias_ruck added inline comments.
test/functional/chronik_ws_avalanche.py
86 ↗(On Diff #55424)

Isn’t this strictly better?

97 ↗(On Diff #55424)

Dito

100 ↗(On Diff #55424)

Lost comprehension seems cleaner here

This revision now requires changes to proceed.Aug 30 2025, 09:10
Fabien added inline comments.Aug 31 2025, 19:47
test/functional/chronik_ws_avalanche.py
86 ↗(On Diff #55424)

You can't have null bytes in python code as a string, that's not supported so I had to use this hack. I will add a comment.

Fabien updated this revision to Diff 55451.Aug 31 2025, 19:49

Rebase, explain the bytes([0]) and use "lost comprehension"

Harbormaster completed remote builds in B34300: Diff 55451.Aug 31 2025, 20:10
tobias_ruck accepted this revision.Sep 2 2025, 06:37
This revision is now accepted and ready to land.Sep 2 2025, 06:37
Closed by commit rABC556df980d5df: [chronik] Improve coverage of avalanche event ws messages (authored by Fabien). · Explain WhySep 2 2025, 06:56
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC556df980d5df: [chronik] Improve coverage of avalanche event ws messages.

Revision Contents
Changeset List

PathSize
test/
functional/
3 lines
331 lines

Diff 55457

View Options

test/functional/chronik_ws.py

Loading...
View Options

test/functional/chronik_ws_avalanche.py

Loading...