vcr: Apply hook in vcr recorder #1349
Conversation
gemmahou
force-pushed
the
vcr-hook
branch
3 times, most recently
from
b048a7b to
9eef7ca
Compare
| @@ -236,6 +240,8 @@ func NewHarness(ctx context.Context, t *testing.T) *Harness { | |||
| kccConfig.GCPAccessToken = h.gcpAccessToken | |||
| } else if targetGCP := os.Getenv("E2E_GCP_TARGET"); targetGCP == "real" { | |||
| t.Logf("targeting real GCP") | |||
| } else if targetGCP := os.Getenv("E2E_GCP_TARGET"); targetGCP == "vcr" { | |||
There was a problem hiding this comment.
Non-blocking future improvement idea: Can we have all the environment variables and valid values used by integration tests documented and maintained somewhere? As someone who rarely touches the code here, it's hard for me to figure out what environment variables I should set and why.
There was a problem hiding this comment.
I created b/332719149 to address this comment.
| ret := inner | ||
| if t := ctx.Value(httpRoundTripperKey); t != nil { | ||
| ret = &http.Client{Transport: t.(http.RoundTripper)} | ||
| if targetGCP := os.Getenv("E2E_GCP_TARGET"); targetGCP == "vcr" { |
There was a problem hiding this comment.
Non-blocking refactoring idea:
Can we do one targetGCP := os.Getenv("E2E_GCP_TARGET") in the beginning of the function, and then do switch case/if else for different conditions in line233-247, line314 and one, etc?
Repetitively retrieving the same environment seems to reduce the readability.
There was a problem hiding this comment.
this will be done in b/332719149 as well.
| if t := ctx.Value(httpRoundTripperKey); t != nil { | ||
| ret = &http.Client{Transport: t.(http.RoundTripper)} | ||
| } | ||
| dir := "pkg/test/resourcefixture/testdata/vcr/cassette/" |
There was a problem hiding this comment.
Just curious: Is cassette a special code name? I personally don't know much about VCR and its concepts so would like to learn more context.
There was a problem hiding this comment.
recorder and cassette are both VCR concepts. Similar to the VCR recorder in the real world, this test package simulates the workflow, recording HTTP interactions instead of audio (😂), and saving them in a file, which is used to replay them later.
TBH, the folder name can be anything (tape, episode, etc., 😂), I just borrowed the name as it's a known concept for storing recorded logs.
|
Thanks @gemmahou /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit 0dc8eef
into
GoogleCloudPlatform:master
Change description
ref: https://github.com/dnaeon/go-vcr?tab=readme-ov-file#hooks
The hook will replace values in request/response pairs before we save them to file.
Note: Applying hook to replace data that we don't want to expose to public. Some data, like timestamp, or id, does not contain sensitive information, so it's fine to leave them for now.
If we can migrate VCR testing as a part of prow job, and save cassette file to GCS bucket, then we may not need hooks since it's a private storage.
Tests you have done
make ready-prto ensure this PR is ready for review.