Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

test: Add Codec trait unit tests #413

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ldiego08
Copy link

@ldiego08 ldiego08 commented Nov 17, 2023

Summary of Changes

  • Add unit tests to codec.rs to assert Codec implementations for the following works as expected:

    • bdk::bitcoin::Amount
    • bdk::bitcoin::Script
    • bdk::bitcoin::secp256k1::ecdsa::RecoverableSignature
    • u64
  • Add anyhow crate to stacks-core for easy error propagation in unit tests.

NOTE: There are some assertions against hex representations of the expected value to keep the tests as readable as possible and to avoid replicating the serialization or deserialization code. Formal snapshot testing using a crate like insta can prove a more scalable alternative asserting between complex values.

Testing

Run tests normally via cargo make test.

Risks

None.

How were these changes tested?

Changes are tests themselves and were verified by running cargo make test.

What future testing should occur?

None.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@CLAassistant
Copy link

CLAassistant commented Nov 17, 2023

CLA assistant check
All committers have signed the CLA.

stacks-core/src/codec.rs Outdated Show resolved Hide resolved
@ldiego08 ldiego08 marked this pull request as ready for review November 17, 2023 15:35
stacks-core/src/codec.rs Outdated Show resolved Hide resolved
stacks-core/src/codec.rs Outdated Show resolved Hide resolved
stacks-core/Cargo.toml Show resolved Hide resolved
@ldiego08 ldiego08 requested a review from CAGS295 November 20, 2023 18:39
}

#[test]
fn should_fail_deserialize_recoverable_signature_with_recovery_id_bytes_out_of_bounds(
Copy link
Author

Choose a reason for hiding this comment

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

The recovery ID can only be 0 - 3 i32. A greater number is out of bounds and should fail to deserialize.

}

#[test]
fn should_fail_deserialize_recoverable_signature_with_signature_bytes_non_ecdsa(
Copy link
Author

Choose a reason for hiding this comment

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

The signature data is ECDSA, hence if the R or S components are out of bounds, deserialization should fail.

stacks-core/Cargo.toml Show resolved Hide resolved
@ldiego08
Copy link
Author

@CAGS295 seems like merging is blocked due to one workflow needing approval. Is there something else I need to do to get this merged?

@CAGS295
Copy link
Contributor

CAGS295 commented Nov 23, 2023

Hi @ldiego08, thank you for your contribution. You need to wait for a core maintainer to approve the workflow. @AshtonStephens

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants