Support digest function in GrpcStore#1548
Conversation
|
Please do not open up PRs that do not adhere to the checklist, unless they do not impact NativeLink. |
6b47cd7 to
8b954aa
Compare
|
The failed check before isn't known what it is failed for seems a deployment failure! Hopefully the deployments succeeded now |
aaronmondal
left a comment
There was a problem hiding this comment.
+@allada @MarcusSorealheis What do we do about test coverage here? The grpcStore currently has no tests 😅 https://tracemachina.github.io/nativelink/coverage/build/source/nativelink-store/src/grpc_store.rs.html
@asr2003 I think it should be fine if we just cover the changed parts, i.e. unit tests for the read, has_with_results and is_supported_digest_function, and only tests that trigger the branches that you added (i.e. tests that force the new error handling and check that they're actually hit).
Reviewable status: 0 of 2 LGTMs obtained, and 0 of 2 files reviewed (waiting on @allada)
|
Sure! I will add tests to ensure the branch coverage. |
c1bf8c1 to
d09fbcc
Compare
|
@aaronmondal @MarcusSorealheis @allada Any update on this? |
d09fbcc to
e8823c7
Compare
e8823c7 to
fd36005
Compare
fd36005 to
d393d25
Compare
d393d25 to
3e9e619
Compare
|
@aaronmondal @MarcusSorealheis @allada - is this expeted to go through? Otherwise may extend the bounty with the testing-extension? |
3e9e619 to
f8f5b8e
Compare
2afa479 to
7bcc71b
Compare
|
@aaronmondal @MarcusSorealheis I have added unit tests as per review comments for the logic added of digests in |
|
@MarcusSorealheis @aaronmondal Would appreciate your review when you have time. |
7bcc71b to
a793073
Compare
Description
ResourceInfoto defaultdigest_functiontosha256if not provided.Fixes #1325
/claim #1325
Type of change
Please delete options that aren't relevant.
How Has This Been Tested?
Please also list any relevant details for your test configuration
Tests added
Checklist
bazel test //...passes locallygit amendsee some docsThis change is