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

[Test-Proxy] Consider adding ability to skip recording request body/entire request #2434

Closed
JoshLove-msft opened this issue Dec 12, 2021 · 10 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library.

Comments

@JoshLove-msft
Copy link
Member

JoshLove-msft commented Dec 12, 2021

In .NET Test Framework, there is a feature where you can skip recording either an entire request, or a request body. This is useful for large request bodies where the content is not meaningful, e.g. form recognizer has a test where the request body is just an empty pdf. For now, I'm using the Test Framework implementation when using the Test Proxy for now, but we should consider moving this into Test Proxy so all languages can use the feature.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 12, 2021
@kurtzeborn kurtzeborn assigned scbedd and JoshLove-msft and unassigned scbedd Jan 3, 2022
@kurtzeborn
Copy link
Member

@JoshLove-msft, looks like you already have a PR for this, so assigning to you.

@kurtzeborn kurtzeborn added Client This issue points to a problem in the data-plane of the library. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Jan 3, 2022
@catalinaperalta
Copy link
Member

@scbedd Python Form Recognizer also needs this, should I open a separate issue?

@scbedd
Copy link
Member

scbedd commented Jan 14, 2022

Nope! It's the same problem. Your +1 is all that's necessary!

@JoshLove-msft
Copy link
Member Author

To be clear, the PR that I had linked was implementing this in the .NET Test Framework in its integration with Test Proxy. I can submit a separate PR that adds this feature to the Test Proxy.

@scbedd
Copy link
Member

scbedd commented Jan 20, 2022

I would really appreciate this Josh, as it's definitely something that the test-proxy needs. If you don't do it, someone (or me) will eventually get around to it 👍 .

@JoshLove-msft
Copy link
Member Author

Sure, my thought was to just have another custom header that we include in the request headers to signify that the body should not be recorded.

@scbedd
Copy link
Member

scbedd commented Jan 21, 2022

Something like x-recording-skip or the like. Yeah, sounds good.

@JoshLove-msft
Copy link
Member Author

We should also add the option to avoid recording the entire test - this is useful for teardown scenarios.
/cc @heaths

@JoshLove-msft
Copy link
Member Author

maybe x-recording-mode:

  • skip-body
  • skip

@heaths
Copy link
Member

heaths commented Feb 4, 2022

In the case of Key Vault, we call using (Recording.DisableRecording()) { } in a few places like with LROs or clean-up to avoid putting a lot of unnecessary entries in JSON recordings. This significantly reduces the size of recordings.

For now, I only need to update a few tests and have commented out our old way, but switching everything over to use a non-instrumented client would take quite a while and, so close to release, I'm not keen on risking test coverage. It might be just fine using a non-instrumented client, but will take time to know for sure and we don't have a lot of time.

Seems like the ability to stop recording some request/responses would be generally useful as well.

ghost pushed a commit that referenced this issue Feb 8, 2022
heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Mar 2, 2022
heaths added a commit to Azure/azure-sdk-for-net that referenced this issue Mar 2, 2022
* Normalize LinkBase for Azure.Core shared files

* Revert changes for Azure/azure-sdk-tools#2434

* Compare recorded bodies

Seems in the course of creating predictable results, I inadvertently fixed #11634.

* Don't enumerate JsonPathSanitizers twice

Fixes #27310
yanfa317 pushed a commit to yanfa317/azure-sdk-for-net that referenced this issue Mar 8, 2022
* Normalize LinkBase for Azure.Core shared files

* Revert changes for Azure/azure-sdk-tools#2434

* Compare recorded bodies

Seems in the course of creating predictable results, I inadvertently fixed Azure#11634.

* Don't enumerate JsonPathSanitizers twice

Fixes Azure#27310
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

5 participants