-
Notifications
You must be signed in to change notification settings - Fork 655
dataconnect: testing: shouldBe for Struct and Value improved efficiency and difference reporting #7601
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
Conversation
…cy and difference reporting
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Code Review
This pull request introduces a comprehensive set of testing utilities for Struct and Value protobufs, significantly improving the efficiency of equality checks and the clarity of difference reporting in tests. The new deepCopy, diff, map, and walk functions are well-designed and robustly tested with property-based tests. The changes to shouldBe matchers to use these new utilities are a great improvement. Overall, this is an excellent contribution that will make testing with these protobuf types much easier and more effective. I've left a few minor suggestions for further improvement.
...e-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoWalk.kt
Outdated
Show resolved
Hide resolved
...se-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoMap.kt
Outdated
Show resolved
Hide resolved
...se-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoMap.kt
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
|
/gemini summary |
|
/gemini review |
Summary of ChangesThis pull request significantly enhances the Highlights
Changelog
Activity
|
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.
Code Review
This pull request introduces a comprehensive set of testing utilities for protobuf Struct and Value types, with a focus on improving equality checking efficiency and providing detailed difference reports. The new functionalities, including deepCopy, diff, map, and walk, are well-organized into separate files. The integration with ProtoTestUtils significantly enhances test failure diagnostics, which is a valuable improvement for developers. The property-based unit tests are thorough and give confidence in the robustness of these new utilities. I have a few minor suggestions to enhance code style and robustness, but overall, this is an excellent and well-executed contribution.
...aconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoValuePath.kt
Show resolved
Hide resolved
...taconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoDeepCopy.kt
Show resolved
Hide resolved
...e-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoDiff.kt
Show resolved
Hide resolved
...se-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoMap.kt
Show resolved
Hide resolved
...se-dataconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoMap.kt
Show resolved
Hide resolved
...aconnect/testutil/src/main/kotlin/com/google/firebase/dataconnect/testutil/ProtoValuePath.kt
Show resolved
Hide resolved
| val mappedValue = toValueProto().map(callback) | ||
| checkNotNull(mappedValue) { | ||
| "callback returned null for root, " + | ||
| "but must be a non-null ${Value.KindCase.STRUCT_VALUE} [qhkdn2b8z5]" |
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.
just curious - what are these seemingly random strings?
stephenarosaj
left a comment
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 - lots over my head but very reassuring to have such good testing infra
This PR introduces a suite of new test utilities for handling Protobuf
StructandValuetypes. These additions improve the efficiency and clarity of testing Protobuf-based data structures by providing deep copying, fast equality comparisons, and detailed difference reporting.Highlights
structFastEqual,listValueFastEqual, andvalueFastEqualfor efficient equality checks, andstructDiff,listValueDiff,valueDifffunctions that provide detailed, path-aware difference reports * Improved ProtoTestUtils Matchers: UpdatedbeEqualTomatchers inProtoTestUtils.ktforStructandValueto leverage the new fast equality checks and include comprehensive difference reports in failure messages, significantly improving test diagnostics.between Protobuf structures.
deepCopyextension functions for ProtobufStruct,ListValue, andValuetypes, enabling the creation of independent, identical copies.ProtoWalk.ktandProtoMap.kt, offeringwalkandmapextension functions forStruct,ListValue, andValueto facilitate recursive traversal and transformation of Protobuf data.ProtoValuePath.ktto includeMutableProtoValuePathand new utility functions for path manipulation and string representation.Changelog
deepCopyextension functions forStruct,ListValue, andValue.structFastEqual,listValueFastEqual,valueFastEqualfor efficient equality checks.structDiff,listValueDiff,valueDifffor detailed difference reporting.Differencesealed interface andDifferencePathPairdata class for managing differences.mapextension functions forStruct,ListValue, andValuefor recursive transformations.beEqualTomatchers forStructandValueto usestructFastEqualandvalueFastEqual.beEqualTomatchers to include detailed diff reporting in failure messages.MutableProtoValuePathtypealias.ProtoValuePathComponentandProtoValuePathfor path manipulation and string representation, includingwithAppendedListIndex,withAppendedStructKey,withAppendedComponent,isStructKey,structKeyOrThrow,toPathString, andappendPathString.walkandwalkValuesextension functions forStruct,ListValue, andValuefor recursive traversal.distinctPairextension function to accept an optionalisEquallambda for custom equality checks.ProtoConvenienceExtsUnitTest.ProtoDeepCopyUnitTest.ProtoDiffUnitTest.ProtoWalkUnitTest.