Add support for writing integration tests that fail #417

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

src/descriptor/tr.rs

+                } 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

integration_test/run.sh

@@ -31,6 +31,7 @@ sleep 3

 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

integration_test/src/test_desc.rs

+// 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

src/descriptor/tr.rs

+                } 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

tests/test_desc.rs

 mod setup;
+// use crate::test_util::{self, TestData};


remove the comment

In ac639f6

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

tests/test_desc.rs

@@ -55,12 +84,30 @@ pub fn test_desc_satisfy(cl: &Client, testdata: &TestData, desc: &str) -> Witnes
        .unwrap();
    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

tests/test_desc.rs

    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

tests/test_desc.rs

    assert!(wit.len() == 3); // control block, script and one signatures

    // 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

src/descriptor/mod.rs

@@ -332,7 +332,7 @@ impl<Pk: MiniscriptKey + ToPublicKey> Descriptor<Pk> {
            Descriptor::Wpkh(ref wpkh) => Ok(wpkh.address(network)),
            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

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

Add support for writing integration tests that fail #417

Add support for writing integration tests that fail #417

Uh oh!

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!

Uh oh!