chore: refactor GrpcConversions to use OffsetDateTime instead of deprecated methods #1415
Conversation
BenWhitehead
commented
May 25, 2022
- Add GrpcConversions#timestampCodec convert between java.time.OffsetDateTime and com.google.protobuf.Timestamp
- Add property test for timestampCodec
- Replace all usage of "Long" methods in GrpcConversions with their appropriate java.time alternatives
google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java
Outdated
Show resolved
Hide resolved
…ecated methods * Add GrpcConversions#timestampCodec convert between java.time.OffsetDateTime and com.google.protobuf.Timestamp * Add property test for timestampCodec * Replace all usage of "Long" methods in GrpcConversions with their appropriate java.time alternatives
| private static final class Supp implements ArbitrarySupplier<Timestamp> { | ||
| @Override | ||
| public Arbitrary<Timestamp> get() { | ||
| return StorageArbitraries.timestamp(); |
There was a problem hiding this comment.
Why did you need this helper static method?
There was a problem hiding this comment.
Rather than the Arbitrary<Timestamp> from StorageArbitraries being registered globally, this method provides the resolution handle to jqwik so it knows how to generate arbitraries for the test method.
Above on line 41 @ForAll(supplier = Supp.class) points to this class as the means to resolve the Arbitrary.
There was a problem hiding this comment.
If we'd rather it be registered globally, I can do that too.
There was a problem hiding this comment.
oh, this is just to test the timestampCodec specifically; wouldn't you need a timestamp supplier for other tests such as bucket/ object metadata? I think that's what is confusing me. This is more of a question that can be answered after PR approval. Thanks Ben
There was a problem hiding this comment.
Other things would also need the timestamp supplier, which so far has been accessed and composed directly via StorageArbitraries#timestamp().
This supplier class is effectively wiring so that jqwik finds that method for the test.
| Codec.of(this::crc32cEncode, this::crc32cDecode); | ||
|
|
||
| @VisibleForTesting | ||
| final Codec<OffsetDateTime, Timestamp> timestampCodec = |
There was a problem hiding this comment.
Why isn't this in our codecs source instead?
There was a problem hiding this comment.
Following the pattern from ApiaryConversions for that conversion to DataTime[1].
Give this conversion is specific to the use of protos it probably makes sense to keep it near the proto conversions. Or, is there another location you'd like to see this moved to?
[1]