Page MenuHomePhabricator
Differential D17638

[chronik] Add an integration test for the blockchain_info endpoint in bitcoinsuite-chronik-client
AcceptedPublic
Actions

Authored by hazzarust on Tue, Feb 4, 23:05.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Depends on D17595 - this diff uses features from D17595 files test_runner.rs and handler.rs.

Use the bitcoinsuite-chronik-client to return the tip height and tip hash length, thus we can compare with the python bitcoin tests and assert if they are equal.

Test Plan

Please set BUILD_DIR env to export BUILD_DIR="/path/to/build_dir
UNIX: ./contrib/teamcity/build-configurations.py build-bitcoinsuite-chronik-client
cd modules/bitcoinsuite-chronik-client && cargo test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
blockchain_info
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32469
Build 64426: Build Diffbuild-bitcoinsuite-chronik-client
Build 64425: arc lint + arc unit

Event Timeline

hazzarust requested review of this revision.Tue, Feb 4, 23:05
hazzarust created this revision.
Harbormaster completed remote builds in B32312: Diff 52524.Tue, Feb 4, 23:11
hazzarust edited the summary of this revision. (Show Details)Tue, Feb 4, 23:11
hazzarust edited the test plan for this revision. (Show Details)
PiRK edited the summary of this revision. (Show Details)Thu, Feb 6, 09:39
PiRK added a parent revision: Restricted Differential Revision.
PiRK added a subscriber: PiRK.Fri, Feb 7, 11:13
PiRK added inline comments.
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
16 ↗(On Diff #52524)

you probably want to use a cross platform function here (maybe std::env::temp_dir()?), just so we don't introduce issues in case we want to run tests on windows some day

Fabien requested changes to this revision.Mon, Feb 10, 11:09
Fabien added a subscriber: Fabien.

See the feedback from D17595: remove the debug statements, and rebase. The suggestion from Pierre is also a good one that you should take into account. Otherwise the state machine looks good to me.

This revision now requires changes to proceed.Mon, Feb 10, 11:09
hazzarust updated this revision to Diff 52693.Thu, Feb 20, 19:34
hazzarust edited the summary of this revision. (Show Details)

Revamped blockchain_info to include the new trait changes.

hazzarust added a comment.EditedThu, Feb 20, 19:36

I will make the changes from Pierre's comment in a separate diff to all current integration tests.

Quoted Text you probably want to use a cross platform function here (maybe std::env::temp_dir()?), just so we don't introduce issues in case we want to run tests on windows some day

hazzarust updated this revision to Diff 52694.Thu, Feb 20, 19:37

lint

hazzarust updated this revision to Diff 52695.Thu, Feb 20, 19:38

sorry relint

Fabien requested changes to this revision.Thu, Feb 20, 19:46

The title and the file name are incorrect, please fix this before the review

This revision now requires changes to proceed.Thu, Feb 20, 19:46
Harbormaster completed remote builds in B32433: Diff 52694.Thu, Feb 20, 19:57
Harbormaster completed remote builds in B32432: Diff 52693.Thu, Feb 20, 20:00
Harbormaster completed remote builds in B32434: Diff 52695.
hazzarust retitled this revision from [chronik] Add blockchain_info to bitcoinsuite-chronik-client to [chronik] Add an integration test for the blockchain_info endpoint in bitcoinsuite-chronik-client.Thu, Feb 20, 22:36
hazzarust edited the summary of this revision. (Show Details)
hazzarust updated this revision to Diff 52696.Thu, Feb 20, 22:36

Renamed blockchain_info_new to blockchain_info

hazzarust edited the summary of this revision. (Show Details)Thu, Feb 20, 22:37
Harbormaster completed remote builds in B32435: Diff 52696.Thu, Feb 20, 22:42
Fabien requested changes to this revision.Fri, Feb 21, 08:45
Fabien added inline comments.
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
24 ↗(On Diff #52696)

it's not testing anything. Call it get_blockchain_info instead

70 ↗(On Diff #52696)

Please add a comment to explain briefly what the step does (mine a block, park a block, invalidate a block etc.)

120 ↗(On Diff #52696)

This should raise an error. Since step 0 is handled in the switch this is expected to be unreachable

This revision now requires changes to proceed.Fri, Feb 21, 08:45
hazzarust updated this revision to Diff 52712.Fri, Feb 21, 14:10

Added unreachable panic, renamed test_blockchain_info to blockchain_info and added comments

Harbormaster completed remote builds in B32447: Diff 52712.Fri, Feb 21, 14:27
Fabien requested changes to this revision.Fri, Feb 21, 18:01

Please make the linter happy

This revision now requires changes to proceed.Fri, Feb 21, 18:01
hazzarust updated this revision to Diff 52723.Sat, Feb 22, 12:39

lint

Harbormaster completed remote builds in B32455: Diff 52723.Sat, Feb 22, 12:59
Fabien accepted this revision.Mon, Feb 24, 08:25
This revision is now accepted and ready to land.Mon, Feb 24, 08:25
Fabien added a comment.Mon, Feb 24, 08:27

Maybe we could actually print to stdout what step is running and what it does instead of the comment, since stdout is captured by default and not shown upon success. But we can do that later if it feels necessary.

Fabien requested changes to this revision.Mon, Feb 24, 08:38
Fabien added inline comments.
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
147 ↗(On Diff #52723)

Please rename, you're not testing the ipc

This revision now requires changes to proceed.Mon, Feb 24, 08:38
Fabien mentioned this in D17687: [Chronik] Add integration test for block_and_blocks endpoint into bitcoinsuite-chronik-client.Mon, Feb 24, 08:43
hazzarust updated this revision to Diff 52729.Mon, Feb 24, 12:47

Rename blockchain_info_ipc

Harbormaster completed remote builds in B32461: Diff 52729.Mon, Feb 24, 12:53
Fabien requested changes to this revision.Mon, Feb 24, 19:41
Fabien added inline comments.
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
66 ↗(On Diff #52729)

Why do you need the line continuation \ here...

95 ↗(On Diff #52729)

Please keep the code consistent and use an assert message everywhere or nowhere

130 ↗(On Diff #52729)

...but not here ? Please keep it consistent

130 ↗(On Diff #52723)

This is not a very good message. The issue is that you got an unexpected extra "ready" message from the setup framework. "Counter" is just the name of a variable, nothing another dev can reason about without knowing the code

This revision now requires changes to proceed.Mon, Feb 24, 19:41
hazzarust updated this revision to Diff 52740.Mon, Feb 24, 20:32

Updated !unreachable() comment and removed assert comments

Fabien requested changes to this revision.Mon, Feb 24, 20:34
Fabien added inline comments.
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
121 ↗(On Diff #52740)

wtf, some lines have \ some don't. \o/
Fix the layout

This revision now requires changes to proceed.Mon, Feb 24, 20:34
Harbormaster completed remote builds in B32468: Diff 52740.Mon, Feb 24, 20:41
hazzarust added inline comments.Mon, Feb 24, 21:52
modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs
121 ↗(On Diff #52740)

This is the linter. I will fix

hazzarust updated this revision to Diff 52741.Mon, Feb 24, 21:58

Updating comment (linter plz dont kill me )

hazzarust updated this revision to Diff 52743.Mon, Feb 24, 22:02

Updated comment

Harbormaster completed remote builds in B32469: Diff 52741.Mon, Feb 24, 22:04
Harbormaster completed remote builds in B32471: Diff 52743.Mon, Feb 24, 22:08
Fabien accepted this revision.Tue, Feb 25, 09:53
This revision is now accepted and ready to land.Tue, Feb 25, 09:53

Revision Contents
Changeset List

PathSize
modules/
bitcoinsuite-chronik-client/
tests/
143 lines

Diff 52741

View Options

modules/bitcoinsuite-chronik-client/tests/blockchain_info.rs

Loading...