-
Notifications
You must be signed in to change notification settings - Fork 140
Fuzz test Thrift to Proto mapper round trips #1457
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
Fuzz test Thrift to Proto mapper round trips #1457
Conversation
…of the file, constants at the top, and then fix the failures of tests/ensure that the tests that are currently unmapped either have exclusions or the mapper itself is fixed
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (82.50%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
internal/compatibility/api_test.go
Outdated
}, | ||
ExcludedFields: []string{ | ||
// Exclude nested fields that have complex issues like in DescribeDomainResponse | ||
"Domain.ActiveClusters", // Nil pointer dereference in mapper conversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a bug or a problem in data representation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This strikes me as really good work, very useful.
Re the todos / fields that couldn't be mapped, I'd request a clarification / distinction between the reasons for finding that fields can't be mapped, something along the lines of:
- This field isn't mapped because it's not relevant and we don't care. it's known, understood and safe to ignore.
- This field isn't mapped, the reason is not understood or known or not investigated yet.
- A clear bug / failure to handle nils / failure to map / a known bug
and so fourth, to be Rumsfeldian for a second just a means for the reader to differentiate between the following dimensions:
- investigated / not investigated
- bug / not a bug / limitation of the mapper / not added because too much work
Other than requesting clarity on the comments, I have no other concerns. And, to be clear, am not asking for further investigations or effort (that's totally fine to push to future work). Just a clear indication of what is currently known
…n/mappertestsfuzz
…to cwarren/mappertestsfuzz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for all your work on this!
What changed?
Adds fuzz tests to all thrift->proto->thrift round trip mappers.
Why?
When mapping from idl to another there are multiple scenarios:
Our existing tests test for scenarios 2 and 3, but don't test for 1 - if someone adds a new field and omits it from the mappers + the test data, it will never be tested for. This PR adds a fuzz test which randomly assigns data to all fields (including nils), which ensures that unmapped data will be tested for without requiring the developer to check it (and keeps the tests for 2 & 3).
However I will say - it's added 2000 lines to this file which makes it borderline unreadable (maybe not borderline).
How did you test it?
It's all unit tests.
Potential risks
N/A