Property testing for define-functions #426

ameeratgithub · 2024-06-20T10:26:20Z

closes #289

codecov · 2024-06-20T10:44:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.54%. Comparing base (13792d0) to head (35f129f).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #426   +/-   ##
=======================================
  Coverage   86.54%   86.54%           
=======================================
  Files          43       43           
  Lines       19192    19192           
  Branches    19192    19192           
=======================================
  Hits        16609    16609           
  Misses       1118     1118           
  Partials     1465     1465

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Acaccia · 2024-06-20T13:07:13Z

clar2wasm/tests/wasm-generation/functions.rs

+
+use crate::{random_expressions, PropValue, TypePrinter};
+
+const DEFINE_PRIVATE_READONLY: [&str; 2] = ["define-private", "define-read-only"];


What is the purpose of this constant?

How is it different from this

I had no idea this existed and yes, this is as bad.

Acaccia · 2024-06-20T13:08:06Z

clar2wasm/tests/wasm-generation/functions.rs

+    #![proptest_config(super::runtime_config())]
+
+    #[test]
+    fn define_private_readonly(


Why would you test two separate Clarity functions in the same test? If the test breaks, how are we suppose to know which of the two functions is wrong?

I think this is similar

yes, it is as bad. No idea what would happen if the test failed.

Acaccia · 2024-06-20T13:09:37Z

clar2wasm/tests/wasm-generation/functions.rs

+            let expr=format!("(begin {expr})");
+            let snippet=&format!(r#"({function} (func (a {})) {expr}) (func {v})"#,v.type_string());


You did the opposite of what should have been tested for a function declaration: you should have generated any number of argument and a simple expression for the body. Your goal is to test function definitions, not begin.

Do you suggest that I should Ignore begin? I mean we should test intermediary expressions though. I'll take care of arguments

You don't test intermediary expressions: begin is an expression. You test a single expression which is a begin.

Acaccia · 2024-06-20T13:23:31Z

clar2wasm/tests/wasm-generation/functions.rs

+            let expr=format!("(begin {expr})");
+            let snippet=&format!(r#"(define-public (func (a {})) {expr}) (func {v})"#,v.type_string());


Exact same issues as the previous test.

See comment above

see reply above

Acaccia · 2024-06-20T13:28:13Z

clar2wasm/tests/wasm-generation/main.rs

+    }
+}
+
+fn ensure_last_response(last_value: PropValue, is_public_function: bool) -> PropValue {


Imagine being someone who didn't write this function. You read it's name, and the function tells you that it ensures that the last expression is a response.

Why last? There is only one argument here that is a Value, why should we specify it's the last of something?

If the second argument is false, the function does nothing! You didn't ensure anything here.

I actually had the idea before reading this function that true would create a ok value and false an err, but not at all. There is no way to create an err.

Good point. Will fix it

Acaccia · 2024-06-20T13:31:41Z

clar2wasm/tests/wasm-generation/main.rs

+    }
+}
+
+fn random_expressions(


Here is the same, imagine being someone who didn't write this. What does random_expression even mean? Why are there 3 return values? limit is limit of what? Why a random expression should care if it is a public function?

Okay there are two approaches. Increase the duplication a bit and or pass some vars to remove duplication. limit is maximum number of expressions being generated.

Don't tell me what limit is: write a comment or make it more explicit!

Acaccia · 2024-06-20T13:34:02Z

clar2wasm/tests/wasm-generation/main.rs

@@ -381,6 +382,63 @@ fn qualified_principal() -> impl Strategy<Value = Value> {
    })
 }

+fn value_to_string(value: &PropValue, to_response: bool) -> String {


Imagine being someone who didn't write this function. Why would we have this function when we know that a PropValue implements Display? Why converting a Value to a String would care if it's a response or not?

This is to ensure that last value is always "stringified" response when function is public. Public functions require last value to be response

Where in the name do you understand that the stringified response is a response for a public function?

Acaccia · 2024-06-20T13:36:43Z

clar2wasm/tests/wasm-generation/functions.rs

+                expected_val
+            );
+    }
+}


(Optional - I hope there is a unit test about it, bud didn't check)

Public functions have another property: if it returns an err, a modification to a datamap should be discarded. This property could also be tested.

ameeratgithub · 2024-06-20T17:07:40Z

@Acaccia could you take a look at it again?

Acaccia

The tests for the functions definitions don't test at all if the arguments are accessible. They should be used in the function body.

There are no tests to prove that any type could be returned from private and read-only functions.
Same for public function, it could show more kind of return types than (response int).

Acaccia · 2024-06-21T11:25:39Z

clar2wasm/tests/wasm-generation/control_flow.rs

@@ -24,16 +24,12 @@ fn begin_strategy() -> impl Strategy<Value = (String, PropValue, bool)> {
                expressions.push(' ');
            }

-            expressions.push_str(&s);
+            expressions.push_str(&format!("{v}"));
        }

        let last_value = values.last().unwrap().clone();


You could simplify this function a lot with the usage of split_last

Simplify code a bit, but repetition of 2 statements. Fixed though

Acaccia · 2024-06-21T11:26:19Z

clar2wasm/tests/wasm-generation/control_flow.rs

@@ -24,16 +24,12 @@ fn begin_strategy() -> impl Strategy<Value = (String, PropValue, bool)> {
                expressions.push(' ');
            }

-            expressions.push_str(&s);
+            expressions.push_str(&format!("{v}"));


No need for a macro here, that's a to_string

I thought macros are faster.

No of course not! Macros add steps in the compilation process to create the code that will be placed here, and then this code still has to be executed in the end.

Acaccia · 2024-06-21T11:30:57Z

clar2wasm/tests/wasm-generation/control_flow.rs

+fn generate_begin_expressions(
+    maximum_expressions: usize,
+) -> impl Strategy<Value = (String, PropValue, bool)> {
+    prop::collection::vec(PropValue::any(), 1..=maximum_expressions).prop_map(move |values| {


If this is used only for begin, shouldn't this function be inlined in the begin test? That's what I said in the previous PR here

I got the idea that you're only interested in name change. I'm not sure what do you think of lengthy functions and separation of concerns. What I thought that building expressions isn't the test's job. I know it's not being used anywhere else but aren't they only tradeoffs?

What separation of concern are you talking about here? It's literally

fn _foo() { // super // long // body } pub fn foo() { _foo() }

Alright. How do you make a longer function readable? I mean there are examples which splits a complex function into smaller functions just to make code readable. Those functions aren't used anywhere. There is one problem with this though. Users debugging the code might lose context if they are too many.

A 20 lines function should normally be readable, nothing special. You can always add comments to describe each sections of your functions if you feel like it could make the reading easier.

Acaccia · 2024-06-21T11:37:04Z

clar2wasm/tests/wasm-generation/functions.rs

+proptest! {
+    #![proptest_config(super::runtime_config())]
+
+    #[test]
+    fn define_public_ok(
+        (arguments, parameters) in generate_random_function_arguments(20)
+    ) {
+            let snippet=&format!("(define-public (func {arguments}) (ok 1)) (func {parameters})");
+
+            crosscheck(
+                snippet,
+                Ok(Some(Value::Response(ResponseData { committed: true, data: Box::new(Value::Int(1)) })))
+            );
+    }
+}
+
+proptest! {
+    #![proptest_config(super::runtime_config())]
+
+    #[test]
+    fn define_public_err(
+        (arguments, parameters) in generate_random_function_arguments(20)
+    ) {
+            let snippet=&format!("(define-public (func {arguments}) (err 1)) (func {parameters})");
+
+            crosscheck(
+                snippet,
+                Ok(Some(Value::Response(ResponseData { committed: false, data: Box::new(Value::Int(1)) })))
+            );
+    }
+}


For me, those tests are completely identical: you define a public function and return a response type.
I fail to see a reason why one of those tests could succeed and one could fail.

Then I failed to understand this

(Optional - I hope there is a unit test about it, bud didn't check)
Public functions have another property: if it returns an err, a modification to a datamap should be discarded. This property could also be tested.

You don't make any datamap modification here. I didn't even realize that was suppose to address this comment.

Alright. A little misunderstanding.

Acaccia · 2024-06-21T11:39:22Z

clar2wasm/tests/wasm-generation/main.rs

                s.push(':');
+                s.push(' '); // Space required after colon in tuple as function argument


should be merged into a push_str

ameeratgithub · 2024-06-21T13:13:53Z

The tests for the functions definitions don't test at all if the arguments are accessible. They should be used in the function body.

There are no tests to prove that any type could be returned from private and read-only functions. Same for public function, it could show more kind of return types than (response int).

I'm not sure I understood this. Consider following example

>> (define-public (func) 1)
<stdin>:1:1: error: public functions must return an expression of type 'response', found 'int'

Acaccia · 2024-06-21T13:26:15Z

@ameeratgithub

Your tests should make sure that all the function's arguments are accessible. I would personally write a function that takes any number of arguments and returns a tuple which uses them all.
-> This test would prove that the function can take any number of arguments of any type and use them all in the function body.

I would also make a function that can take also any number of random arguments and returns a randomly generated value.
-> This test would prove that a function can return any type.

For the public function, you can generate a random response value like this:
response in (prop_signature(), prop_signature()).prop_flat_map(|(ok_ty, err_ty)| PropValue::from_type(TypeSignature::ResponseType(Box::new((ok_ty, err_ty)))))

ameeratgithub · 2024-06-22T12:27:15Z

@Acaccia thanks for the details. Hopefully it will be good now.

ameeratgithub added 4 commits June 15, 2024 13:20

prop tests are added

2f3261d

Merge branch 'main' into test/prop-test-define-functions

da368a8

random expressions added in define functions

69f799f

fix: formatting

2740f50

Acaccia requested changes Jun 20, 2024

View reviewed changes

refactor: random arguments added to define functions

22fb34d

smcclellan requested review from Acaccia and csgui June 20, 2024 17:41

fix: check_type_string for tuple fixed

77e6ff7

Acaccia requested changes Jun 21, 2024

View reviewed changes

refactor: made code cleaner

d2cc0e9

refactor: tests improved

35f129f


		use crate::{random_expressions, PropValue, TypePrinter};

		const DEFINE_PRIVATE_READONLY: [&str; 2] = ["define-private", "define-read-only"];

		let expr=format!("(begin {expr})");
		let snippet=&format!(r#"({function} (func (a {})) {expr}) (func {v})"#,v.type_string());

		let expr=format!("(begin {expr})");
		let snippet=&format!(r#"(define-public (func (a {})) {expr}) (func {v})"#,v.type_string());

		s.push(':');
		s.push(' '); // Space required after colon in tuple as function argument

Property testing for define-functions #426

Are you sure you want to change the base?

Property testing for define-functions #426

Conversation

ameeratgithub commented Jun 20, 2024

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acaccia Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameeratgithub commented Jun 20, 2024

Acaccia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Acaccia Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameeratgithub commented Jun 21, 2024

Acaccia commented Jun 21, 2024

ameeratgithub commented Jun 22, 2024

codecov bot commented Jun 20, 2024 •

edited

Loading

Acaccia Jun 20, 2024 •

edited

Loading

Acaccia left a comment •

edited

Loading

Acaccia Jun 21, 2024 •

edited

Loading