Add support for writing integration tests that fail by kanishk779 · Pull Request #417 · rust-bitcoin/rust-miniscript

kanishk779 · 2022-05-22T15:44:05Z

Earlier we did not have the ability to write integration tests that fail.
The default behaviour was to panic and stop the execution of the program.
But in addition to verifying that the code works in the correct cases, we must also ensure that it should generate an appropriate error in incorrect cases.
Some functions are modified to return Result<T, E> to support this feature.
Four failing tests have also been added (tests 10-13) to demonstrate this.

apoelstra · 2022-05-23T17:37:44Z

+                } else {
+                    return Err(Error::Unexpected(String::from(
+                        "Computing spend data on a valid Tree should always succeed",
+                    )));


How do you actually hit this case? How do you obtain a Tr with an invalid tree? Can you add a unit test?

Relatedly, can you separate out the changes to src/ into a separate commit from the changes to the integration test?

Thanks for your contribution!!

This case is hit when the height of the tree is more than 128. In the Impl for TaprootBuilder in file taproot.rs of the rust-bitcoin project, there is a function insert, which returns a TaprootBuilderError.

For which function do you want me to add the unit tests?

Ok I will split it into two commits.

Can you add a unit test for Taproot::spend_info which causes this new error path to be taken?

I think that would answer my "how do you hit this case" question, which I'm still confused about, sorry.

And thanks for splitting up the commits!

Yep, I am also curious when this gets hit. This seems like a big bug if we need to change this.

sanket1729 · 2022-05-25T20:19:44Z


 RPC_URL=http://localhost:12348 \
    RPC_COOKIE=${TESTDIR}/1/regtest/.cookie \
+    RUST_BACKTRACE=1 \


It is odd to supply this in RPC_URL.
You can supply this via your own shell environment.

sanket1729 · 2022-05-25T20:21:29Z

+// Currently this is not being used, since miniscript::Error is being propagated
+// But if the types of errors grow in future, we can extend this enum and use it.
+#[derive(Debug, PartialEq)]
+pub enum MalleableDescError {


This Error should be used as the return type

Here is a suggestion for how to use this enum.

Rename this type to DescTestError

Change the return type from Error to DescTestError for the test function.

pub enum DesctestError { DescParseError(Error), PsbtFinalizeError(Error), AddressComputationError(Error), ... }

In the test function then, you would change the lines to make sure that correct variant is retuned.

sanket1729 · 2022-05-25T20:23:43Z

+                } else {
+                    return Err(Error::Unexpected(String::from(
+                        "Computing spend data on a valid Tree should always succeed",
+                    )));


Yep, I am also curious when this gets hit. This seems like a big bug if we need to change this.

sanket1729

Left a few comments

sanket1729 · 2022-06-12T18:33:03Z

+use std::fmt;
+use miniscript::Error;
 mod setup;
+// use crate::test_util::{self, TestData};


remove the comment

In ac639f6

sanket1729 · 2022-06-12T18:33:55Z

    assert_eq!(blocks.len(), 1);

-    let desc = test_util::parse_test_desc(&desc, &testdata.pubdata);
+    let mut desc;


use map_err instead

In ac639f6

sanket1729 · 2022-06-12T18:34:04Z

    let derived_desc = desc.derived_descriptor(&secp, 0).unwrap();
    // Next send some btc to each address corresponding to the miniscript
+    let mut rest;
+    match derived_desc.address(bitcoin::Network::Regtest) {


same as above: map_err

In ac639f6

sanket1729 · 2022-06-12T18:34:26Z


    // Test 5: When everything is available, we should select the key spend path
-    let wit = test_desc_satisfy(cl, testdata, "tr(X,{pk(X1),and_v(v:pk(X2),pk(X3!))})");
+    let wit = test_desc_satisfy(cl, testdata, "tr(X,{pk(X1),and_v(v:pk(X2),pk(X3!))})").unwrap();


Fix other warnings requiring the same unwrap

In ac639f6, you should fix all the unwraps

sanket1729 · 2022-06-12T18:34:42Z

            Descriptor::Wsh(ref wsh) => Ok(wsh.address(network)),
            Descriptor::Sh(ref sh) => Ok(sh.address(network)),
-            Descriptor::Tr(ref tr) => Ok(tr.address(network)),
+            Descriptor::Tr(ref tr) => Ok(tr.address(network)?),


Remove this commit

sanket1729 · 2022-06-13T17:13:54Z

@kanishk779 , the history is still unclean. The first commit still adds a bunch of files.

kanishk779 · 2022-06-18T16:23:36Z

@sanket1729 I have made the required changes.

sanket1729

ACK ab79593

…ration tests that fail ab7959382a2ae9e0dd8aaf3500fd3951455ff464 Use DescError instead of miniscript::Error (kanishk779) b446ced71ff8a01de49f0400fa43754f64d4022c Add support for writing integration tests that fail (kanishk779) Pull request description: - Earlier we did not have the ability to write integration tests that fail. - The default behaviour was to panic and stop the execution of the program. - But in addition to verifying that the code works in the correct cases, we must also ensure that it should generate an appropriate error in incorrect cases. - Some functions are modified to return Result<T, E> to support this feature. - Four failing tests have also been added (tests 10-13) to demonstrate this. ACKs for top commit: sanket1729: ACK ab7959382a2ae9e0dd8aaf3500fd3951455ff464 Tree-SHA512: 2f0800194c06325cd32fc81988f88989d412c4d0164d8d3f2105068a16db22a3e25fcdefe9f1c7736739faf765fb1e8adc850f9c524e7dec7c89af4dcca22e47

kanishk779 force-pushed the aman-integration-tests branch 3 times, most recently from 09ed81b to 210376a Compare May 22, 2022 20:24

apoelstra reviewed May 23, 2022

View reviewed changes

sanket1729 reviewed May 25, 2022

View reviewed changes

kanishk779 force-pushed the aman-integration-tests branch from 210376a to 58ccb93 Compare May 29, 2022 14:22

kanishk779 force-pushed the aman-integration-tests branch 2 times, most recently from 987545b to be10ec1 Compare June 11, 2022 06:59

sanket1729 reviewed Jun 12, 2022

View reviewed changes

kanishk779 force-pushed the aman-integration-tests branch from be10ec1 to 9ef6607 Compare June 13, 2022 16:56

kanishk779 force-pushed the aman-integration-tests branch from 9ef6607 to ce0220d Compare June 13, 2022 17:35

kanishk779 added 2 commits June 18, 2022 15:05

Add support for writing integration tests that fail

b446ced

Use DescError instead of miniscript::Error

ab79593

kanishk779 force-pushed the aman-integration-tests branch from ce0220d to ab79593 Compare June 18, 2022 10:05

sanket1729 approved these changes Jun 19, 2022

View reviewed changes

sanket1729 merged commit e0650ab into rust-bitcoin:master Jun 19, 2022

sanket1729 mentioned this pull request Jun 19, 2022

Add Hash256 associated type #434

Merged

Conversation

kanishk779 commented May 22, 2022

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!

sanket1729 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!

sanket1729 commented Jun 13, 2022

Uh oh!

kanishk779 commented Jun 18, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants