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

Introduce the Test.expectFailure function #2468

Conversation

m-Peter
Copy link
Contributor

@m-Peter m-Peter commented Apr 25, 2023

Closes onflow/cadence-tools#108

Description

Introduces a new native function for allowing/expecting a test case to fail/panic, with a specified error message.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #2468 (9eb2fac) into master (c6117d1) will increase coverage by 0.01%.
The diff coverage is 93.58%.

@@            Coverage Diff             @@
##           master    #2468      +/-   ##
==========================================
+ Coverage   78.28%   78.30%   +0.01%     
==========================================
  Files         327      327              
  Lines       72773    72851      +78     
==========================================
+ Hits        56971    57044      +73     
- Misses      13707    13710       +3     
- Partials     2095     2097       +2     
Flag Coverage Δ
unittests 78.30% <93.58%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
runtime/stdlib/test_contract.go 84.46% <93.58%> (+0.93%) ⬆️

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 25, 2023

I will certainly add more tests, I just opened this up in order to bootstrap a conversation around the implementation details and desired functionality 🙏

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

runtime/stdlib/test.go Outdated Show resolved Hide resolved
runtime/stdlib/test.go Outdated Show resolved Hide resolved
runtime/stdlib/test.go Outdated Show resolved Hide resolved
runtime/stdlib/test.go Outdated Show resolved Hide resolved
runtime/stdlib/test.go Outdated Show resolved Hide resolved
runtime/stdlib/test.go Outdated Show resolved Hide resolved
@turbolent turbolent self-assigned this Apr 25, 2023
@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 26, 2023

@turbolent I have added more test cases, and fixed the scenario where a failure is expected, but there is none (946ffae#diff-4709f76365b48c2caca87b859afcc573afb62cb4e6186b9431d2b540972d402fR619)
However, I'm struggling with the test case here (946ffae#diff-74ad93a1435e307865ff8474567598f410879038788126ec263e27c9244edf98R1487). With a local build, the actual tests (in Cadence Testing Framework), works well. Do you have any clue?
P.S: This PR should be merged after #2403, to avoid huge conflicts 😇

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 26, 2023

import Test

pub struct Foo {
    priv let answer: UInt8
    
    init(answer: UInt8) {
        self.answer = answer
    }
    
    pub fun correctAnswer(_ input: UInt8): Bool {
        if self.answer != input {
            panic("wrong answer!")
        }
        return true
    }
}

pub fun testOne() {
    let foo = Foo(answer: 42)
    Test.expectFailure(fun(): Void {
        foo.correctAnswer(42)
    }, errorMessageSubstring: "wrong answer")
}

pub fun testTwo() {
    let foo = Foo(answer: 42)
    Test.expectFailure(fun(): Void {
        foo.correctAnswer(43)
    }, errorMessageSubstring: "wrong answer!")
}

pub fun testThree() {
    let foo = Foo(answer: 42)
    Test.expectFailure(fun(): Void {
        foo.correctAnswer(43)
    }, errorMessageSubstring: "what is wrong?!")
}

And the equivalent test execution results:

~/Dev/forks/flow-cli/cmd/flow/flow-x86_64-linux- test my_expect_failure_tests.cdc

Test results: "my_expect_failure_tests.cdc"
- FAIL: testOne
		Execution failed:
			error: Execution failed:
			error: Expected a failure, but found none.
			  --> 7465737400000000000000000000000000000000000000000000000000000000:14:8
			
			  --> 7465737400000000000000000000000000000000000000000000000000000000:14:8
			
- PASS: testTwo
- FAIL: testThree
		Execution failed:
			error: Expected error message to include: "what is wrong?!". Found: Execution failed:
			error: panic: wrong answer!
			  --> 7465737400000000000000000000000000000000000000000000000000000000:12:12
			
			  --> 7465737400000000000000000000000000000000000000000000000000000000:12:12

@turbolent
Copy link
Member

@m-Peter figured out what's wrong and also refactored the recover logic, but can't push the commits.

Could you please allow commits from maintainers by checking the "Allow edits from maintainers" box? (see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 26, 2023

@turbolent It seems that it's not possible for organization repositories.. See https://github.com/orgs/community/discussions/5634 I am unable to find this option also..

@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 26, 2023

@turbolent I have sent you an invitation as a collaborator, you should be able to do so now 🙏

@turbolent
Copy link
Member

@m-Peter Thank you! Pushed up the fix. The issue was that the panic function was declared for the checker, but not for the interpreter

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for adding this

@m-Peter m-Peter force-pushed the test-contract-add-expect-failure-function branch from d26ce3e to b1fb4c4 Compare April 27, 2023 10:12
@m-Peter
Copy link
Contributor Author

m-Peter commented Apr 27, 2023

@m-Peter Thank you! Pushed up the fix. The issue was that the panic function was declared for the checker, but not for the interpreter

Oh, I see, great catch 💯 💯 💯

I tried locally the newest changes, but it seems that they affect the displayed error messages. For example:

Screenshot from 2023-04-27 12-22-25

I have added a commit to make them less verbose:

Screenshot from 2023-04-27 12-32-17

What do you think about it? I can drop the changes, if you want.
Otherwise, feel free to merge it, and I will later resolve the conflicts for the other PR (#2403) 🙏

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks better, nice!

@turbolent
Copy link
Member

@m-Peter #2403 is now merged, so there are some conflicts now. The additions of this PR basically have to be moved to test_contract.go

@m-Peter m-Peter force-pushed the test-contract-add-expect-failure-function branch from b1fb4c4 to cf7a943 Compare May 3, 2023 10:14
@m-Peter m-Peter requested a review from turbolent May 3, 2023 10:15
@m-Peter m-Peter force-pushed the test-contract-add-expect-failure-function branch from cf7a943 to 246e5ef Compare May 3, 2023 10:18
@m-Peter
Copy link
Contributor Author

m-Peter commented May 3, 2023

@turbolent It was rather impossible to rebase on master, after #2403 was merged. So I had to reset the commits of this PR, manually apply the changes and then force-push. I have added you as co-author though 🙏

Co-Authored-By: Bastian Müller <bastian@axiomzen.co>
@m-Peter m-Peter force-pushed the test-contract-add-expect-failure-function branch from 246e5ef to 9eb2fac Compare May 3, 2023 10:23
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

@turbolent turbolent merged commit e3670a4 into onflow:master May 5, 2023
9 of 14 checks passed
@turbolent turbolent deleted the test-contract-add-expect-failure-function branch May 5, 2023 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Test] Allowing/expecting a test case to fail/panic
3 participants