Add integration tests

[tcharding]

Opening draft PR as a discussion starter on the benefits of having integration tests (see note below).

Add a tests/ directory and to it add integration.rs. Add integration tests for sign, and verify for schnorr sigs and ecdsa sigs (plus recover for ecdsa).

Note: I'm not sure these tests add a lot of value, and they introduce some maintenance burden. Most of this stuff is already tested in example files and example docs code. Do we want integration tests? Is there a better way to go about this? Are these really even integration tests since each test only tests a single module?

[tcharding]

Interesting CI failures, this PR does not touch the tests that fail. Not sure why they are failing?

EDIT: Is it something to do with recovery feature being marked unstable?

[elichai]

Nice!
I think these add a lot of value for testing the public API.
If we had integration tests and required new public API to add a integration test for it we would've catched #344 before it was merged

[RCasatta]

Interesting CI failures, this PR does not touch the tests that fail. Not sure why they are failing?

EDIT: Is it something to do with recovery feature being marked unstable?

Indeed very interesting, it has to do with endianness since I see bytes are swapped every four

  left: `"RecoverableSignature(092e8898c36dedf4fc439e65af0c1e77921f0ba604772b6f2147741f6673ffad09b68c898d12eed0a39aae06ff2080c4025e709f80120ef852e0ada84c1a971601)"`,
 right: `"RecoverableSignature(98882e09f4ed6dc3659e43fc771e0cafa60b1f926f2b77041f744721adff7366898cb609d0ee128d06ae9aa3c48020ff9f705e02f80e1280a8ade05216971a4c01)"`', src/ecdsa/recovery.rs:334:9

[elichai]

The failure is interesting, on master:

$ cross test --target powerpc-unknown-linux-gnu test_debug_output

running 1 test
test key::test::test_debug_output ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 52 filtered out; finished in 0.69s

applying the following patch:

diff --git a/Cargo.toml b/Cargo.toml
index 2db1078..d165928 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -37,7 +37,7 @@ serde = { version = "1.0", default-features = false, optional = true }
 
 
 [dev-dependencies]
-rand = "0.6"
+secp256k1 = { path = ".", features = ["rand", "bitcoin_hashes", "recovery", "global-context"]}
 rand_core = "0.4"
 serde_test = "1.0"
 bitcoin_hashes = "0.10"

now:

$ cross test --target powerpc-unknown-linux-gnu test_debug_output

running 2 tests
test ecdsa::recovery::tests::test_debug_output ... FAILED
test key::test::test_debug_output ... ok

failures:

---- ecdsa::recovery::tests::test_debug_output stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `"RecoverableSignature(092e8898c36dedf4fc439e65af0c1e77921f0ba604772b6f2147741f6673ffad09b68c898d12eed0a39aae06ff2080c4025e709f80120ef852e0ada84c1a971601)"`,
 right: `"RecoverableSignature(98882e09f4ed6dc3659e43fc771e0cafa60b1f926f2b77041f744721adff7366898cb609d0ee128d06ae9aa3c48020ff9f705e02f80e1280a8ade05216971a4c01)"`', src/ecdsa/recovery.rs:334:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    ecdsa::recovery::tests::test_debug_output

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 62 filtered out; finished in 0.70s

[elichai]

Ok, nvm, this was me being stupid.
it always failed, it's just that this change actually enabled all of rust-secp256k1 features.
same happens with: cross test --target powerpc-unknown-linux-gnu test_debug_output --features=recovery

So first we need to fix the CI to run the tests with more combinations of features, also in the cross tests.

About the failing test, the bug is once again that we're relying on the internal representation of libsecp256k1's structs. which are not defined and non portable (so libsecp actually writes integers into those buffers, so they'll differ according to endianess)

[thomaseizinger]

Note: I'm not sure these tests add a lot of value, and they introduce some maintenance burden. Most of this stuff is already tested in example files and example docs code. Do we want integration tests? Is there a better way to go about this? Are these really even integration tests since each test only tests a single module?

My 4 cents:

• Tests in tests/ are nice because they test the public API of a crate
• Doctests to the same thing though, they also only have access to the public API
• For the reasons above, I wouldn't bother too much with the taxonomy of integration vs unit tests
• I think we should prefer extensive doc tests over tests in tests/ because they add to the documentation

[tcharding]

I think we should prefer extensive doc tests over tests in tests/ because they add to the documentation

This makes sense. Examples in rustdocs are slightly harder to maintain (mainly because my tooling doesn't work quite as nicely with them cough syntax highlighting).

Unless there is some objection, I'll attempt to convert the tests I've added here into docs examples.

Thanks

[RCasatta]

Added an issue #375 for the bug with recovery features

[tcharding]

Closing in favour of using docs examples instead of the tests/ directory. Initial PR in this work is here