feat: Add fuzz test for FirestoreDatabase #3340
Conversation
pkg/fuzztesting/fuzzkrm.go
Outdated
| @@ -141,6 +141,9 @@ func (f *FuzzTest[ProtoT, KRMType]) Fuzz(t *testing.T, seed int64) { | |||
| t.Fatalf("error mapping from krm to proto: %v", ctx.Err()) | |||
| } | |||
|
|
|||
| // Remove any output only or known-unimplemented fields | |||
| fuzz.Visit("", p2.ProtoReflect(), nil, clearFields) | |||
There was a problem hiding this comment.
Not sure why we need to clear fields again. cc @acpana
There was a problem hiding this comment.
This controller takes an opinion on the values that should be set for type, app_engine_integration_mode, and deletion_protection_state. Values for those fields are set in the ToProto method, regardless of what is passed in. Therefore, the test fails if the "unimplemented fields" are not erased in the result object.
There was a problem hiding this comment.
This behavior seems resource-specific, but is currently applied library-wide, affecting all resources. Ideally we want to scope it down to individual resource fuzz tests. A potential solution is to add something like a defaultFields to the KRMTypedFuzzer struct. The defaultFields can be set at resource level, and be cleared in library after p2 is calculated. It also helps distinguish between unimplemented fields vs. fields that will be defaulted by the controller.
There was a problem hiding this comment.
Just to clarify, those fields are not settable in the KRM resource. They are given hardcoded, default values in order to make the resource declarative. Not sure if this is a common scenario. Though, is there any reason we would not want this behavior (of clearing unimplemented fields) for all resources?
There was a problem hiding this comment.
Though, is there any reason we would not want this behavior (of clearing unimplemented fields) for all resources?
My understanding is that it defeats the purpose of roundtrip. For example,
- The KRM -> proto conversion is incorrectly adding data to ignored / unimplemented field.
- or, the proto -> KRM conversion is converting data to the wrong field that then gets mapped back to ignored field (I am thinking about in a spec fuzz test where the mapper functions were incorrectly converting proto field to a status field).
Although this is likely a rare edge case, we should ideally catch it when it happens.
If a proper fix is not straightforward, we can merge this PR as is and add a TODO for later, to unblock other PRs.
There was a problem hiding this comment.
My understanding is that it defeats the purpose of roundtrip.
Got it, makes sense. Thanks for the examples. I will try to figure out a better solution for this.
There was a problem hiding this comment.
Ok, I have fixed this @jingyih. I moved the defaulting behavior out of the mapper and into a separate function, and marked all the "default value" fields as unimplemented (in the fuzz test). This organization is probably better for maintainability / readability anyway, and makes it so the defaulting behavior does not affect the fuzz test.
jasonvigil
force-pushed
the
firestoredatabase-fuzz
branch
from
deef648 to
fcd31c5
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit 763fe18
into
GoogleCloudPlatform:master
Seems there were quite a few fields added to FirestoreDatabase since it was originally implemented.