Add ChronikClientNode object to communicate with in-node instances of the chronik indexer. Initialize with method to conduct getblockchaininfo API call. Replace NNG chronik-client in integration test with ChronikClientNode.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC9f73786d1fea: [chronik-client] Add in-node ChronikClientNode object and handle standard…
Same as D14915
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- in-node-chronik-client-first-endpoint
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 25951 Build 51477: Build Diff chronik-client-integration-tests · chronik-client-tests Build 51476: arc lint + arc unit
Event Timeline
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 13 ↗ | (On Diff #43263) | layout (here and everywhere) |
| 177 ↗ | (On Diff #43263) | I'm happy to learn that BCH, Ergon and Lotus have a built-in chronik as well. |
| modules/chronik-client/test/testNode.ts | ||
| 110 ↗ | (On Diff #43263) | chronik-native.fabien.cash has it, but it doesn't matter because you will not use a live API call for your tests, right ? |
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 13 ↗ | (On Diff #43263) | I'm not sure what the issue is here -- don't have */ on the smae line as last line of comment? |
| modules/chronik-client/test/testNode.ts | ||
| 110 ↗ | (On Diff #43263) | I'm keen to start using the in-node chronik server for alias-server and ecash-herald as soon as possible -- so, would like to get a published version of chronik-client up before diving into the the task of refactoring all the unit tests to run with py scripts. I agree that's a better way to do it, but I don't think it should hold up the availability of in-node chronik-client access (tested to the same standard as existing nng chronik-client) |
Comment Actions
chronik-native.fabien.cash has it, but it doesn't matter because you will not use a live API call for your tests, right ?
Requesting an exception for this.
Getting the python unit tests in as soon as possible is the right way to do it. The reason I think it's acceptable to get a working version published using the existing test approach: we are feature matching the existing chronik-client for NNG to the in-node chronik. The old one has always used live unit tests.
I think the benefit of having a working chronik-client for in-node chronik as soon as possible (tested to the same standard as the current chronik-client) is worth the extra work of getting these unit tests in even though we plan to replace them with python scripts.
There is a prod impact on delaying the availability of in-node chronik-client that imo is worth this extra effort. The extra effort is trivial: refactoring the existing fixtures from the nng unit tests to have the in-node structure. The benefit is a more robust alias-server available before launch.
Comment Actions
Now that we have a functional test framework ready for the js client, you should add the endpoints little by little together with integration tests.
Comment Actions
I would rather have this mark on CI than you posting a screenshot for every single diff :)
Comment Actions
Requesting change for the constructor, not the CI that will require some work
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 20 ↗ | (On Diff #43547) | Since this is new code, it's a good idea to not introduce the single string at all and create no tech debt from the start |
Comment Actions
do not support string type for Node object
| modules/chronik-client/test/integration/blockchain_info.ts | ||
|---|---|---|
| 3–5 ↗ | (On Diff #43547) | these changes are applied when I save this file on my machine. So, it's a formatting thing that should be added to arclint digging into what it is exactly, not sure if this is prettier or eslint |
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (chronik-client-tests) failed.
Tail of the build log:
/work/modules/chronik-client /work/abc-ci-builds/chronik-client-tests > chronik-client@0.9.0 prepublish > npm run build > chronik-client@0.9.0 build > tsc test/integration/blockchain_info.ts(92,47): error TS2345: Argument of type 'string' is not assignable to parameter of type 'string[]'. test/integration/blockchain_info.ts(98,47): error TS2345: Argument of type 'string' is not assignable to parameter of type 'string[]'. test/integration/blockchain_info.ts(104,47): error TS2345: Argument of type 'string' is not assignable to parameter of type 'string[]'. test/integration/blockchain_info.ts(110,47): error TS2345: Argument of type 'string' is not assignable to parameter of type 'string[]'. test/integration/blockchain_info.ts(116,47): error TS2345: Argument of type 'string' is not assignable to parameter of type 'string[]'. npm ERR! code 2 npm ERR! path /work/modules/chronik-client npm ERR! command failed npm ERR! command sh -c -- npm run build npm ERR! A complete log of this run can be found in: npm ERR! /root/.npm/_logs/2023-12-12T17_47_05_273Z-debug-0.log Build chronik-client-tests failed with exit code 2
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 15 ↗ | (On Diff #43578) | The doc still needs update |
Comment Actions
doc patch
| modules/chronik-client/src/ChronikClientNode.ts | ||
|---|---|---|
| 15 ↗ | (On Diff #43578) | ...no idea what happened here https://reviews.bitcoinabc.org/D14851?vs=43547&id=43577  mb since I am pushing these manually with arc diff --update ... anyway weird, will push this again |
Comment Actions
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
Comment Actions
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.