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

Add raft tests; backwards compatible time.Time #13

Merged
merged 3 commits into from
Dec 2, 2022
Merged

Add raft tests; backwards compatible time.Time #13

merged 3 commits into from
Dec 2, 2022

Conversation

swenson
Copy link

@swenson swenson commented Nov 30, 2022

We are concerned with binary backwards compatibility for this project, especially as the upstream has a history of making breaking changes.

With that in mind, I collected a bunch of data from https://github.com/hashicorp/raft for v0.5.5, v1.1.5, and v1.1.6 of go-msgpack to ensure that, moving forward, we can always decode old raft logs.

For the tests to pass, we have to support decoding time.Time using the encoding/binary marshalling format, which was used in certain v1.1.6.

Also added in the go.sum file, which is needed to build with newer versions of Go.

We are concerned with binary backwards compatibility for this
project, especially as the upstream has a history of making breaking
changes.

With that in mind, I collected a bunch of data from
https://github.com/hashicorp/raft for v0.5.5, v1.1.5, and v1.1.6
of `go-msgpack` to ensure that, moving forward, we can always decode
old raft logs.

For the tests to pass, we have to support decoding `time.Time`
using the `encoding/binary` marshalling format, which was used
in certain v1.1.6.

Also added in the `go.sum` file, which is needed to build with
newer versions of Go.
@swenson swenson requested review from tomhjp and kisunji November 30, 2022 18:22
@banks
Copy link
Member

banks commented Dec 1, 2022

I absolutely love this! This library has caused so much pain over the years just fixing this one issue that caused us to diverge is fantastic.

I love the testing methodology too. I have a couple of suggestions that might make them cleaner to read and maintain:

  1. All the binary examples are great but make the tests pretty hard to read. I think in Consul we've been slowly moving large example-based tests like this to use testdata instead where each example payload is a separate file in ./testdata and then the actual test code can be much more easily read and worked on.
  2. I wonder if, in addition to these we could add fuzz tests that randomly populate each struct type and then try the full matrix of encode/decode pairs. I've used go-fuzz for this a bunch recently. I did submit a PR for it recently related to how it fuzzes time.Time though so might be worth ensuring that fix is in the version used. This could also be a later PR along with possibly other test structs with field types we don't have in raft?

Just som suggestions. the actual PR here looks awesome already!

@swenson
Copy link
Author

swenson commented Dec 1, 2022

@banks excellent ideas! I'll work on simplifying the tests, and this is great excuse for me to learn the fuzzing framework in the latest versions of Go. :)

@banks
Copy link
Member

banks commented Dec 1, 2022

Go's recent fuzzing improvement are awesome, but they probably won't help for this type of test yet as they can only fuzz a few simple types. go-fuzz on the other hand uses reflection to populate any arbtrary nested struct which is great for this style of test still. Here's an example: https://github.com/hashicorp/raft-wal/blob/main/codec_test.go#L20 p.s. the time comment in that test was merged upstream now too so might not be needed any more if they released that or you use head.

@swenson
Copy link
Author

swenson commented Dec 1, 2022

@banks I moved all of the verbose test data to separate files in internal/testdata. I left them as Go files since they need to keep the type information, and it makes them slightly easier to read/write.

@swenson
Copy link
Author

swenson commented Dec 1, 2022

(I think I'll do fuzzing in a separate PR)

Copy link

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

I didn't have any of the historical context for this issue, but I've dug around to educate myself and this looks good to me AFAICT. The tests are great 👍

go.mod Show resolved Hide resolved
codec/internal/testdata/raft.go Outdated Show resolved Hide resolved
@swenson
Copy link
Author

swenson commented Dec 2, 2022

Thanks!

@swenson swenson merged commit 249f0f2 into main Dec 2, 2022
@swenson swenson deleted the raft-tests branch December 2, 2022 18:16
Copy link

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

Late to the party but looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants