Skip to content
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

vcr: Apply custom matcher #1372

Merged
merged 6 commits into from
Mar 25, 2024

Conversation

gemmahou
Copy link
Collaborator

Change description

Add custom matcher to match incoming request against saved request

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@justinsb justinsb added this to the 1.115 milestone Mar 15, 2024
@gemmahou gemmahou force-pushed the vcr-matcher branch 2 times, most recently from 3872857 to 114fc17 Compare March 18, 2024 07:13
// Use both indexes to get the string contains the message we need to remove
temp := s[startIndex+len(keyword) : startIndex+len(keyword)+endIndex]
// Replace
s = strings.Replace(s, temp, "fake debug info", -1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just remove it, IIUC it only shows up for (some) googlers / callers. So for outside contributors, it might just be confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to remove this this whole code block in #1393.

To speed up the test, I'd like to remove unnecessary interactions from the yaml file. For example, remove failed HTTP logs with error codes 404/400/403 and repeated logs that come from the retry transport.

I left a TODO here stating that the code will be removed in an upcoming PR.

tests/e2e/unified_test.go Outdated Show resolved Hide resolved
@justinsb
Copy link
Collaborator

Some nits, but this PR looks to have addressed some of the things that confused me in the other (WIP) PR.

So:
/approve
/lgtm

I would hold this for you to sequence as you prefer, because I've lost track of the sequencing, but it needs a rebase anyway.

Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

New changes are detected. LGTM label has been removed.

@gemmahou
Copy link
Collaborator Author

Code review comments have been addressed.
Manually add lgtm label and merge this PR.

@gemmahou gemmahou added the lgtm label Mar 25, 2024
@google-oss-prow google-oss-prow bot merged commit 523d79a into GoogleCloudPlatform:master Mar 25, 2024
8 checks passed
@gemmahou gemmahou deleted the vcr-matcher branch March 25, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants