primitives: Add an API test module by tcharding · Pull Request #4078 · rust-bitcoin/rust-bitcoin

tcharding · 2025-02-19T03:44:31Z

In preparation for 1.0-ing primitives add an api test module that makes an effort to verify the API surface.

This is similar to what is in units and what is in development for hashes (in #4017).

Note, there is a WIP attempt at this in #3992.

Close: #3928

tcharding

ooph, this was pending from yesterday.

tcharding · 2025-02-20T04:18:35Z

primitives/tests/api.rs

+    // If this builds then traits are dyn compatible.
+    struct Traits {
+        // a: Box<dyn block::Validation>,
+    }


Hey @Kixunil I went digging for your simplified suggestion and I couldn't find it. Flagging that I wasn't ignoring you my memory is just failing me right now.

Just define an #[allow(unused)] function that takes _: &dyn Trait.

yancyribbens · 2025-03-20T12:45:20Z

primitives/tests/api.rs

+// `Debug` representation is never empty (C-DEBUG-NONEMPTY).
+#[test]
+fn api_all_non_error_types_have_non_empty_debug() {
+    macro_rules! check_debug {


would not_empty be an accurate name also?

or debug_not_empty anyway can also just ignore my comment here.

yancyribbens · 2025-03-20T12:47:12Z

primitives/tests/api.rs

+        transaction.compute_wtxid();
+        transaction.version;
+        Witness::arbitrary(&mut u).unwrap();
+        // ad: witness::Iter<'a>,


why the ad comment here?

Oh this is a hangover from when there were more than 26 and I used ad as a variable name. Your comment made me remember that I forgot to fix all the local var names after removing a bunch of stuff yesterday (during rebase). Thanks

yancyribbens · 2025-03-20T12:52:48Z

primitives/tests/api.rs

+    // We abuse `Arbitrary` here to get a quick and dirty instance.
+    let ab: [u8; 32] = [0xab; 32];
+    let mut u = arbitrary::Unstructured::new(&ab);
+    let transaction = Transaction::arbitrary(&mut u).unwrap();


should this belong in checked_debug!? Interesting idea to abuse Arbitrary like this to create random test data, although is this just testing Arbitrary? It doesn't appear to be testing that debug is not empty.

This variable is used below.

yancyribbens · 2025-03-20T12:56:14Z

primitives/tests/api.rs

+//!
+//! The point of these tests are to check the API surface as opposed to test the API functionality.
+//!
+//! ref: <https://rust-lang.github.io/api-guidelines/about.html>


Maybe it's more worthwhile to call out what section(s) of the api-guidelines this is addressing.

There are comments in the code eg C-DEBUG

tcharding · 2025-03-20T21:38:31Z

Thanks for the review. Rebased and fixed single letter variable names.

apoelstra · 2025-03-24T21:46:18Z

primitives/tests/api.rs

+    a: absolute::LockTime,
+    b: relative::LockTime,
+    // block::Checked, // Empty enums are not constructable.
+    // block::Unchecked,


In d845272:

We should uncomment these. They don't need to be constructible for the API tests.

Now resolved.

primitives/tests/api.rs

apoelstra

ACK c80cff3; successfully ran local tests

tcharding · 2025-05-08T20:59:20Z

I had a few review suggestion.

There is no obvious reason why the witness iterator cannot be cloned.

In preparation for 1.0-ing `primitives` add an `api` test module that makes an effort to verify the API surface. This is similar to what is in `units` and what is in development for `hashes` (in rust-bitcoin#4017).

tcharding · 2025-05-08T23:10:53Z

Rebased and feature gated the whole test on hex and arbitrary (already has alloc).

apoelstra

ACK e2d9a8a; successfully ran local tests

apoelstra · 2025-05-09T12:28:20Z

Part of me thinks we should have non-hex non-arbtrary API tests, but realistically that's a lot of boilerplate code for very little value -- these feature gates typically enable entire types at once and none of them gate things like Copy or Clone or constructibility, which is what these API tests are testing.

github-actions bot added test C-primitives labels Feb 19, 2025

tcharding marked this pull request as draft February 19, 2025 03:46

tcharding force-pushed the 03-19-primitives-api branch from 70978e7 to e576550 Compare February 20, 2025 04:17

tcharding mentioned this pull request Feb 20, 2025

primitives: Make the crate conform to the API guidelines #3827

Open

53 tasks

tcharding commented Feb 20, 2025

View reviewed changes

tcharding force-pushed the 03-19-primitives-api branch from e576550 to 8341c3f Compare March 20, 2025 03:59

tcharding marked this pull request as ready for review March 20, 2025 03:59

tcharding force-pushed the 03-19-primitives-api branch from 8341c3f to b6bf31c Compare March 20, 2025 05:01

tcharding mentioned this pull request Mar 20, 2025

api: Add a test for trait dyn compatibility #3829

Closed

3 tasks

yancyribbens reviewed Mar 20, 2025

View reviewed changes

tcharding force-pushed the 03-19-primitives-api branch from b6bf31c to d845272 Compare March 20, 2025 21:38

tcharding mentioned this pull request Mar 21, 2025

units: Add check empty debug macro #4080

Closed

apoelstra reviewed Mar 24, 2025

View reviewed changes

primitives/tests/api.rs Show resolved Hide resolved

tcharding force-pushed the 03-19-primitives-api branch 2 times, most recently from ccf93cf to c80cff3 Compare April 11, 2025 00:07

apoelstra previously approved these changes Apr 14, 2025

View reviewed changes

tcharding dismissed apoelstra’s stale review via 4759942 May 8, 2025 20:58

tcharding force-pushed the 03-19-primitives-api branch from c80cff3 to 4759942 Compare May 8, 2025 20:58

tcharding added 2 commits May 9, 2025 08:59

primitives: Derive Clone on witness::Iter

8ec2d35

There is no obvious reason why the witness iterator cannot be cloned.

primitives: Add an API test module

e2d9a8a

In preparation for 1.0-ing `primitives` add an `api` test module that makes an effort to verify the API surface. This is similar to what is in `units` and what is in development for `hashes` (in rust-bitcoin#4017).

tcharding force-pushed the 03-19-primitives-api branch from 4759942 to e2d9a8a Compare May 8, 2025 23:10

apoelstra approved these changes May 9, 2025

View reviewed changes

apoelstra merged commit 79a6840 into rust-bitcoin:master May 9, 2025
24 checks passed

Conversation

tcharding commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented May 8, 2025

Uh oh!

tcharding commented May 8, 2025

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tcharding commented Feb 19, 2025 •

edited

Loading