Confirm that we don't trivially leak scorecards in any kind of finalize #6288
Conversation
| @@ -509,6 +513,10 @@ func TestHandleEventWithWorkspaceStatusBeforeStarted(t *testing.T) { | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, "abc123", invocation.CommitSha) | |||
| assert.Equal(t, inspb.InvocationStatus_COMPLETE_INVOCATION_STATUS, invocation.InvocationStatus) | |||
|
|
|||
| metrics, err := te.GetMetricsCollector().ReadCounts(ctx, "hit_tracker/"+testInvocationID) | |||
There was a problem hiding this comment.
I think since we are creating "fake" builds by maually streaming BES events, we won't have any cache requests associated with the build, so this assertion would pass even if we didn't flush correctly (because the cache request data in the MetricsCollector will be empty)
The testenv should have a cache client available as te.GetPooledByteStreamClient(), so it might work to create a dedicated test that opens a BES stream, uploads a blob (cachetools.UploadBlob) then downloads the blob (with the invocation ID set in RequestMetadata, so that it gets associated with the BES stream - can use bazel_request.WithRequestMetadata for the DownloadBlob ctx), then making assertions on the counts before/after calling finalize
Modified tests to additionally confirm that we don't leak scorecards just because of a disconnect or retry.
Related issues: N/A