stacks-network / clarity-wasm

`clar2wasm` is a compiler for generating WebAssembly from Clarity.
GNU General Public License v3.0
12 stars 12 forks source link

prop-tests for begin function added #424

Closed ameeratgithub closed 3 months ago

ameeratgithub commented 3 months ago

This PR adds some property tests for begin function.

closes #293

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.54%. Comparing base (6c1a9cf) to head (aa2c2f3). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #424 +/- ## ========================================== + Coverage 86.52% 86.54% +0.01% ========================================== Files 43 43 Lines 19167 19192 +25 Branches 19167 19192 +25 ========================================== + Hits 16584 16609 +25 Misses 1118 1118 Partials 1465 1465 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Acaccia commented 3 months ago

I fundamentally disagree with @csgui ’s comment. The purpose of the tests here is to validate the begin function, not try or assert.

csgui commented 3 months ago

@Acaccia My intention was not ask to test try or assert. Those were just examples to illustrate how begin evaluates intermediate functions.

ameeratgithub commented 3 months ago

@Acaccia I get your point. If we don't need to check/test intermediary expressions then It's just one simple test. Want me to remove these?

Something like this could work though, it would be just one simple test

(define-private (foo (a int)) (+ a 10)) (begin (foo 10))
Acaccia commented 3 months ago

@csgui ah okay, I misunderstood, my bad.

Acaccia commented 3 months ago

@ameeratgithub no, you should test with a random number of random intermediary expressions. That’s the entire point of begin, wrapping several expressions together and returning the value of the last one.

ameeratgithub commented 3 months ago

Alright. I added random expressions, but I'm unwrapping intermediary response expressions recursively, as Brice explained. Unwrapping responses shouldn't cause problems in this case because values are known. @Acaccia

Acaccia commented 3 months ago

@ameeratgithub I don't think we should unwrap responses. We should make sure that the expected value is an error if an intermediate expression is a response, and check that this is consistent with the interpreter.

ameeratgithub commented 3 months ago

Fixed. Sorry I wasn't available yesterday. @Acaccia

ameeratgithub commented 3 months ago

Alright. Now I'm creating only 10 values.

Acaccia commented 3 months ago

Still, I would be curious for the average proportion of is_response_intermediary. Depending on the result, we might need two tests with and without intermediary responses, don’t you think?

ameeratgithub commented 3 months ago

@Acaccia can you please approve if it's okay now?