Details

Reviewers

bytesofman
PiRK

Group Reviewers

Restricted Project

Commits

rABCdb4369859f39: [chronik-client] Add support for integration tests

Summary

This is an initial framework to run chronik-client integration tests against the built-in chronik version.
This leverage the existing test framework so setup scripts can be easily developed, and a state machinebased on IPC communication with nodejs makes it possible to construct complex scenarios.

The exit code is success only if both the python and the JS returns 0, so assertions in the python setup are possible.

If run directly, it is possible to interact with the python script by sending commands from the terminal. This enables testing the setup script standalone.

Integration with the CI will be done in a follow-up.

Test Plan

Build the node with chronik:

mkdir buildChronik
pushd buildChronik
cmake -GNinja .. -DBUILD_BITCOIN_CHRONIK=ON
ninja

Run the setup script standalone:

./test/functional/test_runner.py setup_scripts/chronik-client_blockchain_info

Type the following sequence in the terminal to see the setup steps:

next
next
next
next
stop

Then run the integration tests:

popd
pushd modules/chronik-client
npm ci
BUILD_DIR="${PWD}/../../buildChronik" npm run integration-tests
popd

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Dec 5 2023, 17:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 5 2023, 17:18

Harbormaster completed remote builds in B25834: Diff 43410.Dec 5 2023, 17:19

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_test.ts(23,1): error TS2531: Object is possibly 'null'.
test/integration_test.ts(46,28): error TS2339: Property 'chronik' does not exist on type 'string | number | bigint | true | object'.
  Property 'chronik' does not exist on type 'string'.
test/integration_test.ts(47,31): error TS2339: Property 'chronik' does not exist on type 'string | number | bigint | true | object'.
  Property 'chronik' does not exist on type 'string'.
test/integration_test.ts(51,28): error TS2339: Property 'status' does not exist on type 'string | number | bigint | true | object'.
  Property 'status' does not exist on type 'string'.
test/integration_test.ts(51,46): error TS2339: Property 'status' does not exist on type 'string | number | bigint | true | object'.
  Property 'status' does not exist on 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-05T17_19_37_667Z-debug-0.log
Build chronik-client-tests failed with exit code 2

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 15, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 15, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 15, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'
====== Bitcoin ABC functional tests with the next upgrade activated: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 15, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py
Bitcoin ABC functional tests with the next upgrade activated: chronik-client_basic.py

Integrate with mocha

Harbormaster completed remote builds in B25843: Diff 43431.Dec 6 2023, 12:11

Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 16, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 16, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 16, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'
====== Bitcoin ABC functional tests with the next upgrade activated: chronik-client_basic.py ======

------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/chronik-client_basic.py", line 16, in <module>
    NODEIPCFD = int(os.environ["NODE_CHANNEL_FD"])
  File "/usr/lib/python3.9/os.py", line 679, in __getitem__
    raise KeyError(key) from None
KeyError: 'NODE_CHANNEL_FD'

Each failure log is accessible here:
Bitcoin ABC functional tests: chronik-client_basic.py
Bitcoin ABC functional tests with the next upgrade activated: chronik-client_basic.py

Ready for review

Fabien published this revision for review.Dec 8 2023, 14:07

Fabien retitled this revision from [chronik] Integration tests draft to [chronik-client] Add support for integration tests.

Fabien edited the summary of this revision. (Show Details)

Fabien edited the test plan for this revision. (Show Details)

bytesofman requested changes to this revision.Dec 8 2023, 14:26

bytesofman added a subscriber: bytesofman.

bytesofman added inline comments.

modules/chronik-client/test/integration/blockchain_info.ts
78 ↗	(On Diff #43520)	if this is async, `await testRunner.send('stop')` alt: remove `async` from the `after` desc
86 ↗	(On Diff #43520)	same as above -- if `async`, then `await testRunner.send('next');` if not, remove `async` from the `afterEach`
93 ↗	(On Diff #43520)	not clear why this is `200` -- is this always how the test node spins up? could give it a JS const, `const NODE_STARTUP_EXPECTED_HEIGHT = 200`
99 ↗	(On Diff #43520)	then, would make sense to expect `NODE_STARTUP_EXPECTED_HEIGHT + 10`
105 ↗	(On Diff #43520)	same
111 ↗	(On Diff #43520)	same
117 ↗	(On Diff #43520)	same
test/functional/setup_scripts/ipc.py
27 ↗	(On Diff #43520)	`timeout` is a var here, the input param? I'm not seeing the `1s` definition
test/functional/setup_scripts/setup_framework.py
9 ↗	(On Diff #43520)	what's `noqa` ?
test/functional/test_framework/test_framework.py
145 ↗	(On Diff #43520)	how does this impact other tests that use this framework?

This revision now requires changes to proceed.Dec 8 2023, 14:26

Fabien added inline comments.Dec 8 2023, 14:43

modules/chronik-client/test/integration/blockchain_info.ts
93 ↗	(On Diff #43520)	By default the regtest starts with a height 0, and the node builds an initial chain of 200 blocks. This can be overriden in the test setup with `self.setup_clean_chain = True` in the `set_test_params()` method, so it's the default but not a standard.
test/functional/setup_scripts/ipc.py
27 ↗	(On Diff #43520)	This timeout is the 4th param of the select.select() call below, it's local to this function (wait for 1s for stdin to have data). The timeout variable applies to the whole receive function. I will clarify the comment.
test/functional/setup_scripts/setup_framework.py
9 ↗	(On Diff #43520)	It tells the flake8 linter to ignore the issues from the file. There is a rule that enforce the imports to be the first code lines, and we cannot respect that rule due to the directory structure: we need to add the parent directory to the python modules path before importing. The pathmagic.py file has 2 benefits: It avoids repeating the noqa on all import lines It avoids importing os and sys in the setup scripts when it's not needed (they are still indirectly imported but it's more clear as they're not used by the script).
test/functional/test_framework/test_framework.py
145 ↗	(On Diff #43520)	For the regular tests, `_run_test_internal()` is an alias for `run_test()` so it's a no-op. It's only overridden by the SetupFramework so it can step through the generator.

Remove unused async, make the regtest height a constant, improve comments

Harbormaster completed remote builds in B25899: Diff 43520.Dec 8 2023, 14:47

Harbormaster completed remote builds in B25900: Diff 43523.Dec 8 2023, 15:20

bytesofman accepted this revision.Dec 8 2023, 19:45

bytesofman added inline comments.

modules/chronik-client/test/integration/blockchain_info.ts
4 ↗	(On Diff #43523)	Edge case that NNG chronik-client is able to work with in-node chronik for this particular endpoint, since the proto here is unchanged from NNG to in-node. practically speaking, I think this diff (setting up test env, template for how to do future tests, everything ready to extend to in-node chronik-client) is the best way forward. So -- we have everything in place now to test a running in-node chronik. But the only ChronikClient that exists in this module is still the NNG one. For the `/blockchain-info` endpoint, the data structure happens to be the same -- so an NNG chronik-client happens to be able to connect with an in-node chronik instance, and still get expected info in the expected shape. So this test proves that the testing environment works and proves this unintended edge-case usage of NNG chronik-client. The first diff I put in after this would Put in the ChronikClientNode class with standard API endpoints Replace `ChronikClient` in this file with `ChronikClientNode` -- I don't think we should keep tests that prove unintended use of NNG client add necessary other tests for other endpoints (probably multiple diffs)
test/functional/setup_scripts/ipc.py
29 ↗	(On Diff #43523)

This revision is now accepted and ready to land.Dec 8 2023, 19:45

PiRK added a subscriber: PiRK.Dec 9 2023, 13:16

PiRK added inline comments.

test/functional/setup_scripts/setup_framework.py
20 ↗	(On Diff #43523)	OK. The alternative would have been to add this framework class in the metaclass' whitelist (`if not clsname in ["BitcoinTestFramework", "SetupFramework"]:`)
test/functional/test_framework/test_framework.py
134 ↗	(On Diff #43523)

Looks good. I would just add a comment or a docstring to BitcoinTestFramework._run_test_internal to explain the purpose of the alias and avoid someone unaware of the setup framework accidentally "optimizing" it away.

Fabien added inline comments.Dec 9 2023, 13:39

test/functional/setup_scripts/setup_framework.py
20 ↗	(On Diff #43523)	I considered this but this is leaking the SetupFramework into the test framework. This solution is purely additive otoh so I liked it better
test/functional/test_framework/test_framework.py
134 ↗	(On Diff #43523)	Good call