Page MenuHomePhabricator
Differential D17638

[chronik] Add an integration test for the blockchain_info endpoint in bitcoinsuite-chronik-client
Needs RevisionPublic
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
SeverityLocationCodeMessage
Auto-Fixmodules/bitcoinsuite-chronik-client/src/handler.rs:54WHITESPACE1Found trailing whitespace(s).
Auto-Fixmodules/bitcoinsuite-chronik-client/src/handler.rs:80WHITESPACE1Found trailing whitespace(s).
Unit
No Test Coverage
Build Status
Buildable 32434
Build 64357: Build Diffbuild-bitcoinsuite-chronik-client
Build 64356: 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

Revision Contents
Changeset List

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

Diff 52695

View Options

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

Loading...