Skip to content

units: Add integration test of API surface#3639

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:11-19-units-test-api-surface
Nov 25, 2024
Merged

units: Add integration test of API surface#3639
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:11-19-units-test-api-surface

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Nov 19, 2024

In an effort to check off items in the Rust API guidelines checklist (#3632) add an integration test file that tests:

  • The location of re-exports for various typical usage styles.
  • Regressions in the API surface (things being accidentally moved).
  • All public types implement Debug (C-DEBUG).
  • For all non-error types:
    • Debug representation is never empty (C-DEBUG-NONEMPTY)
  • For all error types:
    • Derive standard traits as defined by rust-bitcoin policy.
  • All data structures implement serde traits (C-SERDE).

I used the cargo check-api script we have laying around from ages ago (#2986) to parse units and get a list of the public types.

@github-actions github-actions bot added C-units PRs modifying the units crate test labels Nov 19, 2024
@tcharding tcharding marked this pull request as draft November 19, 2024 00:16
@tcharding tcharding force-pushed the 11-19-units-test-api-surface branch from 30b101e to 88084dc Compare November 19, 2024 00:17
@tcharding
Copy link
Copy Markdown
Member Author

My dream here is that @apoelstra and I get this one solid then someone else does the same thing for the other crates. @jamillambert would you be wiling to take a stab at this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this up from Enums in case there are more tests we would like to do on just structs (eg Constructors are static, inherent methods (C-CTOR))

@jamillambert
Copy link
Copy Markdown
Contributor

My dream here is that @apoelstra and I get this one solid then someone else does the same thing for the other crates. @jamillambert would you be wiling to take a stab at this?

Yes I can work on this once units is done. I assume you want io and hashes done next.

@tcharding
Copy link
Copy Markdown
Member Author

Yep, io and hashes in this repo and also hex and ordered in other repos - then we can do primitives.

@tcharding
Copy link
Copy Markdown
Member Author

Tagging @apoelstra incase you don't look at this because its draft, chasing feedback please.

@apoelstra
Copy link
Copy Markdown
Member

Concept ACK.

As for the FIXME, I don't think you can usefully test destructors like this. Obviously if you construct a default instance of an object, destructing it will not fail. The question is whether every API-accessable instance of the object can be infallibly destructed. And for that we pretty-much need to manually read Drop impls.

In an effort to check off items in the Rust API guidelines
checklist (rust-bitcoin#3632) add an integration test file that tests:

- The location of re-exports for various typical usage styles.
- Regressions in the API surface (things being accidentally moved).
- All public types implement Debug (C-DEBUG).
- For all non-error types:
    - `Debug` representation is never empty (C-DEBUG-NONEMPTY)
- For all error types:
    - Derive standard traits as defined by `rust-bitcoin` policy.

I used the `cargo check-api` script we have laying around from ages
ago (rust-bitcoin#2986) to parse `units` and get a list of the public types.
@tcharding tcharding force-pushed the 11-19-units-test-api-surface branch from 88084dc to 7b8369f Compare November 25, 2024 02:33
@tcharding tcharding marked this pull request as ready for review November 25, 2024 02:33
@tcharding
Copy link
Copy Markdown
Member Author

Removed the Drop attempt. Also I found that we do have a constructor for amount::Display.

Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7b8369f; successfully ran local tests

@apoelstra
Copy link
Copy Markdown
Member

@tcharding should I merge this? Or do you want to iterate any further?

@tcharding
Copy link
Copy Markdown
Member Author

Lets merge. @jamillambert maybe wait a few days before starting on the other crates so you don't have to go back if/when I find more things we can put in this api test file. I have been over the list of API items a few times already but I have not opened every link and read them so I may still find more.

@apoelstra apoelstra merged commit f27a788 into rust-bitcoin:master Nov 25, 2024
@tcharding tcharding deleted the 11-19-units-test-api-surface branch November 26, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-units PRs modifying the units crate test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants