Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Test] Allowing/expecting a test case to fail/panic #108

Closed
m-Peter opened this issue Apr 21, 2023 · 3 comments · Fixed by onflow/cadence#2468
Closed

[Test] Allowing/expecting a test case to fail/panic #108

m-Peter opened this issue Apr 21, 2023 · 3 comments · Fixed by onflow/cadence#2468

Comments

@m-Peter
Copy link
Contributor

m-Peter commented Apr 21, 2023

Issue To Be Solved

Currently, there is no way to specify that a test is expected to fail/panic. For example:

flow test --cover MerkleProof_test.cdc
Running tests...

Test results: "MerkleProof_test.cdc"
- PASS: testVerifyValidProof
- FAIL: testVerifyInvalidProof
		Execution failed:
			error: panic: invalid proof
			  --> ./contracts/MerkleProof.cdc:12:16
			
- PASS: testVerifyProofWithLowerLeaf
- PASS: testVerifyWithHigherLeaf
Coverage: 100.0% of statements

For the testVerifyInvalidProof test case, the error: panic: invalid proof is the expected program behavior, however the test case is marked with a FAIL status.

Suggest A Solution

Add a built-in method/matcher, which wraps a method call execution and expects a panic with a certain error message.

Test.assertFailsWith(fun(): Void {
    merkleProof.verifyProof(
        proof: [proof.decodeHex()],
        root: root.decodeHex(),
        leaf: leaf.decodeHex(),
        hasherRawValue: 1
    )
}, "invalid proof")

Context

onflow/developer-grants#148

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 21, 2023

cc @SupunS @turbolent
Correct me if I'm wrong, but other than running this through a script and verifying that the ResultStatus is failed, I didn't find any other way. (the script workaround applies only to integration tests, for unit tests this remains impossible)

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 24, 2023

I have come up with a draft for the following behavior:

➜  flow-merkle-proof git:(main) ✗ ~/Dev/forks/flow-cli/cmd/flow/flow-x86_64-linux- test --cover MerkleProof_test.cdc
Running tests...

Test results: "MerkleProof_test.cdc"
- PASS: testVerifyValidProof
- FAIL: testVerifyInvalidProof
		Execution failed:
			error: Expected error message to include: "not what I expected!". Found: Execution failed:
			error: panic: invalid proof
			  --> ./contracts/MerkleProof.cdc:12:16
			
			  --> 7465737400000000000000000000000000000000000000000000000000000000:21:8
			
- PASS: testVerifyProofWithLowerLeaf
- PASS: testVerifyWithHigherLeaf
Coverage: 100.0% of statements

➜  flow-merkle-proof git:(main) ✗ ~/Dev/forks/flow-cli/cmd/flow/flow-x86_64-linux- test --cover MerkleProof_test.cdc
Running tests...

Test results: "MerkleProof_test.cdc"
- PASS: testVerifyValidProof
- PASS: testVerifyInvalidProof
- PASS: testVerifyProofWithLowerLeaf
- PASS: testVerifyWithHigherLeaf
Coverage: 100.0% of statements

@SupunS
Copy link
Member

SupunS commented Apr 24, 2023

Add a built-in method/matcher, which wraps a method call execution and expects a panic with a certain error message.

Test.assertFailsWith(fun(): Void {
    merkleProof.verifyProof(
        proof: [proof.decodeHex()],
        root: root.decodeHex(),
        leaf: leaf.decodeHex(),
        hasherRawValue: 1
    )
}, "invalid proof")

This sounds like a nice solution! 👌

Maybe the name of the function could be something like expectFailure().

Going one step further, we could probably make the second parameter accept another string-matcher (rather than a string), so we could also support asserts like contains/startsWith/etc., for the error message. But of course, doesn't need to be in the first iteration, maybe a future improvement.

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

Successfully merging a pull request may close this issue.

3 participants