Add conformance tests for ignore_empty #126
Conversation
| Message: &cases.IgnoreEmptyProto2ScalarOptionalWithDefault{Val: proto.Int32(42)}, | ||
| Expected: results.Success(true), |
There was a problem hiding this comment.
The fact that this and the case above on line 39 have the same outcome makes this ambiguous: is this passing because the explicit value is greater than zero (per validation rule) or is it passing because it the default value is considered "empty" and so the validation rule is skipped?
If it is meant to be the latter, a non-conformance implementation could still pass the conformance test case due to the former. So I think this needs to be reformulated to better suss out non-conforming implementations.
There was a problem hiding this comment.
Since optional is nullable, it's a noop for this rule regardless. There's a "nonzero" rule above that is neither the default and not zero that captures that case. I will add tests with a negative value which I think is what will cover the remaining missing range of behavior.
There was a problem hiding this comment.
Said another way setting a default doesn't matter for this rule.
Following #124, this adds conformance tests that ensure the
ignore_emptyrules is consistent withrequired. Mainly, if a field cannot differentiate between unset and the zero value (i.e., is not nullable), then this rule applies; effectively this is just repeated and map fields, as well as non-optional proto3 scalar outside of a oneof.Running against
protovalidate-go, a few places whereignore_emptyshould be a noop ends up disabling evaluations. This will be a minor follow-up fix in that library. Patches for the other libraries will follow to bring them into conformance with these tests and those in #124.