zksecurity / noname

Noname: a programming language to write zkapps
https://zksecurity.github.io/noname/
161 stars 35 forks source link

chore: improves tests negative #99

Closed vuvoth closed 1 week ago

vuvoth commented 1 month ago

Changes

katat commented 1 month ago

This simplifies the code a lot, thanks! But can we just use function instead of macro to achieve the same?

vuvoth commented 1 month ago

This simplifies the code a lot, thanks! But can we just use function instead of macro to achieve the same?

We can. However, we need create the function warps this function with #[test] macros for every test.

katat commented 1 month ago

Hmm, yeah. But I think it is fine to use #[test] as it is a common practice for testing, and it is more explicit for code reading.

We are trying to avoid creating custom macros as much as possible, since they are generally harder to comprehend.

vuvoth commented 1 month ago

Got it! So I will update PR. Btw I'm planing to add more tests for packages.

vuvoth commented 1 month ago

Hi @katat! We can't pass something like ErrorKind::ReturnTypeMismatch(..) to function params. I also think the macro is not so hard to read in this case.

mimoo commented 1 month ago

I agree with @katat here that macros should be sort of used as last case resort as they're hard to maintain/update, I don't think the changes are impactful enough here to warrant the introduction of a macro

vuvoth commented 1 month ago

Hey @mimoo, @katat! Please review my PR again!

katat commented 1 month ago

Thanks!

I have two comments:

Can we remove the error stack trace if the test passes? I think these error logs is confusing especially when the tests pass.

It should be more appropriate to have these sort of tests in the type_checker/mod.rs, as it is testing the type checking.

mimoo commented 1 week ago

closing for now, feel free to reopen