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

Remove Golden Tests from the test suite #8162

Open
vmg opened this issue May 20, 2021 · 4 comments
Open

Remove Golden Tests from the test suite #8162

vmg opened this issue May 20, 2021 · 4 comments
Labels

Comments

@vmg
Copy link
Collaborator

vmg commented May 20, 2021

Since we've upgraded our ProtoBuf package version in #8075, the new serializer to JSON and ProtoText inserts random whitespace on its output that prevents byte-wise comparison.

Our test suite makes extensive use of "golden tests" that compare these outputs. In order to complete the upgrade successfully, I had to disable the randomness in the serializer.

This is explained in detail in the following comment:

// DisableProtoBufRandomness disables the random insertion of whitespace characters when
// serializing Protocol Buffers in textual form (both when serializing to JSON or to ProtoText)
//
// Since the introduction of the APIv2 for Protocol Buffers, the default serializers in the
// package insert random whitespace characters that don't change the meaning of the serialized
// code but make byte-wise comparison impossible. The rationale behind this decision is as follows:
//
// "The ProtoBuf authors believe that golden tests are Wrong"
//
// Fine. Unfortunately, Vitess makes extensive use of golden tests through its test suite, which
// expect byte-wise comparison to be stable between test runs. Using the new version of the
// package would require us to rewrite hundreds of tests, or alternatively, we could disable
// the randomness and call it a day. The method required to disable the randomness is not public, but
// that won't stop us because we're good at computers.
//
// Tracking issue: https://github.com/golang/protobuf/issues/1121
//
//go:linkname DisableProtoBufRandomness google.golang.org/protobuf/internal/detrand.Disable

Of course, this is actually a hack. The ideal solution here would be making sure that no tests in our test suite are actually comparing bytes directly.

There are two ways to get there:

  1. Adding helpers to the test suite that parse the contents of the serialized PB and then comparing the parsed structures directly, instead of comparing the bytes.
  2. Normalizing the contents of the serialized PB (i.e. by removing all whitespace) before we do the literal comparison.

The ideal approach will probably change for each test. There are many tests to fix, but fortunately this is something that can be done incrementally. This is a tracking issue so we can get there.

@vmg vmg self-assigned this May 20, 2021
@kaveeshadinamidu
Copy link

Hey, @vmg do you working on this issue?

@vmg
Copy link
Collaborator Author

vmg commented Feb 15, 2022

I am not, we've made some intermittent progress on this but there's still a long trail of tests that need to be fixed.

@exitflynn
Copy link

hi @vmg i'd like to give this a go! can you share about the progress that has been made / any other info that may be of relevance here? 🤠

@vmg
Copy link
Collaborator Author

vmg commented Feb 8, 2023

Hey @exitflynn! This issue is still very much up for grabs. The easiest way to figure out which tests would start failing because of PB comparisons is disabling the hack in go/hack/detrand.go and seeing which tests start failing. From there, it's just a matter of coming up with a good approach (which will probably vary between tests) to compare these Protobuf objects without direct text comparison.

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

No branches or pull requests

3 participants