ferranbt / fastssz

Fast Ethereum2.0 SSZ encoder/decoder
MIT License
74 stars 44 forks source link

sszgen: refactor tests to use golang test runner #117

Closed lightclient closed 4 months ago

lightclient commented 1 year ago

This PR makes it possible to verify the output of sszgen using go test ./sszgen. It also supports a unified diff of mismatching test results:

--- FAIL: TestGolden/alias_array_size.go (0.00s)
main_test.go:57:
--- got
+++ want
@@ -26,12 +26,12 @@
 func (c *Case6) UnmarshalSSZ(buf []byte) error {
    var err error
    size := uint64(len(buf))
-   if size != 64 {
+   if size != 32 {
            return ssz.ErrSize
    }

    // Field (0) 'A'
-   copy(c.A[:], buf[0:64])
+   copy(c.A[:], buf[0:32])

    return err
 }

I'd like to add some more tests here and I think this format is a bit easier to write tests in and maintain. Please let me know what you think!

lightclient commented 1 year ago

I think there are a few benefits to 2) over the current CI check:

ferranbt commented 1 year ago

Not sure if we are on the same page regarding testcases. The encoded code for each case (_encoding.go) is not the expected result of the generated code that gets manually created every time, but it is generated with a make command (generate-testcases). They are only meant to ensure that there are no unexpected code generation changes since they would be visible during PR review.

So, I am not sure what extra security benefit you get by checking that [case]_encoding.go matches the generation of [case].go. You are only ensuring that you did not forget to run the make command.

it's clearer to contributors that there are unit tests because they are defined in a more standard way

Not sure what unit tests you mean? testcases are not unit tests. They are only meant to ensure that there are no unexpected code generation changes.

you can verify the results without needing to bother with checking git

Unsure also about the results? The only thing that the new test checks is that you did no forgot to run the code generation.

failed tests spit out a unified diff so that you can immediately see how the test case is failing

Same as the previous point. I do not need to know there is a difference. I only need a signal that I forgot to run the code generation.