Compare bodies in playback tests #11624
Conversation
| /// <param name="sanitizer">The test record sanitizer.</param> | ||
| public StorageRecordMatcher(RecordedTestSanitizer sanitizer) | ||
| : base(sanitizer) | ||
| public StorageRecordMatcher() |
There was a problem hiding this comment.
So this is already doing its own body comparison to determine whether or not we need to re-record. How does that fit with your vision here? Should we gut this type since we're now doing body comparisons in a more formal way?
Also, can we pass false to the base .ctor for the time being and file an issue about turning it back on? There are a lot of features being developed in other branches and I don't want to randomize them.
There was a problem hiding this comment.
I think the IsEquivalent check that allows us to avoid unneeded re-recording should stay as it covers way more cases than byte-by-byte strict comparison here.
Also, can we pass false to the base .ctor for the time being and file an issue about turning it back on? There are a lot of features being developed in other branches and I don't want to randomize them.
Done
There was a problem hiding this comment.
Maybe I'm misinterpreting @tg-msft's comment here, but does the system already determine if sessions need re-recording, or is that what he's suggesting? It would be nice if it did, but to date I've not seen that behavior and instead have been relying on committing files I know need to change, or robocopying all recordings over and checking if anything of substance changed in the git diffs.
There was a problem hiding this comment.
There is a way to specify if re-recording is required but it's not plug-and-play as other framework features, it needs per library customization most of the time to know what's not important.
There was a problem hiding this comment.
Yeah, the test framework uses your RecordMatcher to determine whether the new record is consistent with what's already on disk so we're not always updating the recording files every time an ETag changes. I've got additional logic for Storage to ignore volatile fields in bodies, but it's super clunky because we have to support XML and JSON at the same time.
There was a problem hiding this comment.
Aren't the files in artifacts/bin/{AssemblyName}/Debug/netstandard2.0/SessionRecords always recorded? I knew about the matcher, but thought it was up to us to determine which to copy. If there's something that will avoid writing out new files, I'd love to learn more. I've just never seen this behavior in KV or Search.
|
/azp run net - search - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run net - storage - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -29,6 +29,8 @@ public partial class CertificateClientLiveTests : CertificatesTestBase | |||
| { | |||
| public CertificateClientLiveTests(bool isAsync, CertificateClientOptions.ServiceVersion serviceVersion) : base(isAsync, serviceVersion) | |||
| { | |||
| // TODO: https://github.com/Azure/azure-sdk-for-net/issues/11634 | |||
There was a problem hiding this comment.
The Key Vault body differences aren't really something we can solve since we're generating keys on the server (will always be different). We could write our own matcher, but I imagine this problem will be common. Could we instead consider an approach that allows us to specify what should be ignored. For example, for JSON (most of our body content across the clients) we could specify JSONPaths. A comparer could do a recursive JSON object comparison and ignore anything specified by the JSONPath (or some filter). Idea for a future iteration, but seeing if anyone is open to it.
There was a problem hiding this comment.
I think it's a good idea that we should try.
We've got em, let's use them