Add coverage for operations on primitive types and introduce JSON schemas#14
Conversation
Also change values to better titles and adapt runner to use both title and filename
stringintech
left a comment
stringintech
left a comment
There was a problem hiding this comment.
Left some notes/questions that I would appreciate the reviewers input on.
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
Concept ACK. Having coverage for operations on primitive types significantly reduces the amount of unit tests each binding needs to implement, when they use this project.
Didn't review the json spec in too much detail, but format seems sensible. Always nice to have things more structured, just slightly worried too much verbosity slows down development velocity, but i suppose with AI tooling that's not really a concern anymore.
Updated stickies-v/py-bitcoinkernel#42 without significant issues (except the >80 bytes header one, which I think should be skipped).
stringintech
force-pushed
the
jsonschema
branch
3 times, most recently
from
b8427ae to
cf0e26f
Compare
|
Thank you for the review @stickies-v! |
|
LGTM. I was on the fence about the JSON schemas initially, but I think the fact that it better allows ensuring that the documentation matches the tests you ship is pretty cool, and worth the complexity. Didn't review in detail, but tests are green now in stickies-v/py-bitcoinkernel#42 without having to make any awkward changes. |
|
Force pushed to address #14 (comment) and drop the last commit (cf0e26f) in order to address #15 in a separate PR. Tested pre-release |
New suites added for operations on block, block hash, block header, script pubkey, transaction, transaction input, transaction output and txid.
Chain suite refactored to return ref for `btck_block_tree_entry_get_block_hash` test cases and call `btck_block_hash_to_bytes` for asserting its value.
Existing suites refactored to call destroy as soon as objects (refs) no longer needed and use `"expected_response": {}` where both `result` and `error` are expected to be null.
stringintech
force-pushed
the
jsonschema
branch
2 times, most recently
from
c3dbed0 to
eccebaf
Compare
janb84
left a comment
janb84
left a comment
There was a problem hiding this comment.
Concept ACK eccebaf
Looks good, doesn't need much to get working on BitcoinKernel.NET.
Of course I like the addition of schemas and schema validaton.
The generated methods-spec.md i think will be a helpful reference.
Solid improvements!
|
Force pushed (compare) to add the missing Tested pre-release |
Adds suite-schema.json which is used to validate each test suite in testdata dir against. Adds a separate schema file per method which is referenced by the root schema file suite-schema.json. Response schema files are also introduced separately (and referenced by their parent method schema file). This allows the runner to later only embed the response schemas for handler response validation. A validator command line tool is written in cmd/suite-validate which performs the testdata suites validation against the root schema and depends on a third party library (github.com/santhosh-tekuri/jsonschema/v6). Co-authored-by: janb84 <githubjanb.drainer976@passmail.net>
Now the runner binary embeds the response schema files and uses the corresponding method response schema for a more strict validation of an incoming response from handler.
Adds a generator in cmd/specgen that deterministically generates a markdown file documenting methods based on method schema files. Now handle-spec points to the generated document using the generator.
|
Thanks for your review @janb84! Marked |
…create` functions 570a627 kernel: assert invalid buffer preconditions in `btck_*_create` functions (stringintech) Pull request description: The kernel API appears to use `nullptr` returns to report failures that callers may reasonably want to recover from: malformed serialized input, object construction failures, chainstate/load failures, and similar runtime conditions. The raw create-function buffer checks seem to be a different case. A failure of `ptr == nullptr && len > 0` does not indicate malformed input data or a failure encountered while deserializing or constructing the requested object. Returning `nullptr` for these checks widens the recoverable error surface with cases that are better treated as programmer errors, similar to other asserted preconditions in this API such as invalid indices and impossible enum/flag states. This change switches those buffer argument checks from `nullptr` returns to assertions in `btck_transaction_create`, `btck_script_pubkey_create`, `btck_block_create`, `btck_block_header_create`, and `btck_chainstate_manager_options_create`. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes). These functions still return `nullptr` when the provided bytes cannot be parsed or when object creation fails during processing. I ended up looking at this while working on the `kernel-bindings-tests` spec/schema for `btck_script_pubkey_create`, where treating this path as a regular error did not seem like the right contract: stringintech/kernel-bindings-tests#14 (comment). ACKs for top commit: stickies-v: ACK 570a627 janb84: ACK 570a627 w0xlt: ACK 570a627 sedited: ACK 570a627 Tree-SHA512: 064d834abe0c27245a144e5290bbeeb510daf9e4d50bb3a8e50bd8a0bf897b3dcf6ad5acfcabf1d8110da120e5e014ee3aea0241c0f181a21c6f3c14dc452ade
…btck_*_create` functions 570a627 kernel: assert invalid buffer preconditions in `btck_*_create` functions (stringintech) Pull request description: The kernel API appears to use `nullptr` returns to report failures that callers may reasonably want to recover from: malformed serialized input, object construction failures, chainstate/load failures, and similar runtime conditions. The raw create-function buffer checks seem to be a different case. A failure of `ptr == nullptr && len > 0` does not indicate malformed input data or a failure encountered while deserializing or constructing the requested object. Returning `nullptr` for these checks widens the recoverable error surface with cases that are better treated as programmer errors, similar to other asserted preconditions in this API such as invalid indices and impossible enum/flag states. This change switches those buffer argument checks from `nullptr` returns to assertions in `btck_transaction_create`, `btck_script_pubkey_create`, `btck_block_create`, `btck_block_header_create`, and `btck_chainstate_manager_options_create`. `btck_block_header_create` additionally asserts the pre-existing documented length contract (must be 80 bytes). These functions still return `nullptr` when the provided bytes cannot be parsed or when object creation fails during processing. I ended up looking at this while working on the `kernel-bindings-tests` spec/schema for `btck_script_pubkey_create`, where treating this path as a regular error did not seem like the right contract: stringintech/kernel-bindings-tests#14 (comment). ACKs for top commit: stickies-v: ACK 570a627 janb84: ACK 570a627 w0xlt: ACK 570a627 sedited: ACK 570a627 Tree-SHA512: 064d834abe0c27245a144e5290bbeeb510daf9e4d50bb3a8e50bd8a0bf897b3dcf6ad5acfcabf1d8110da120e5e014ee3aea0241c0f181a21c6f3c14dc452ade (cherry picked from commit 19e4533)
3 participants
Details included in commit descriptions.
Tested pre-release
v0.0.4-alpha.1in stringintech/go-bitcoinkernel#12 (comment).Thank you @janb84 for your earlier work on JSON schemas in #10. I worked on the structure a bit to enforce stricter test suite validation (according to my earlier #11 (comment)) and making it easier to integrate the schemas for runtime handler response validation and method spec generation.