Add live test coverage for Cosmos endpoints and mitigate issues#13560
Add live test coverage for Cosmos endpoints and mitigate issues#13560christothes merged 5 commits intoAzure:masterfrom
Conversation
ghost
added
the
sdk/tables/Azure.Data.Tables/src/Extensions/TableClientExtensions.cs
Outdated
Show resolved
Hide resolved
| @@ -15,7 +15,8 @@ public override string SanitizeVariable(string variableName, string environmentV | |||
| { | |||
| return variableName switch | |||
| { | |||
| TablesTestEnvironment.PrimaryKeyEnvironmentVariableName => string.Empty, | |||
| TablesTestEnvironment.PrimaryStorageKeyEnvironmentVariableName => string.Empty, | |||
There was a problem hiding this comment.
If you are sanitizing the entire value chances are you don't need to record it.
public string PrimaryCosmosAccountKey => GetOptionalVariable(PrimaryCosmosKeyEnvironmentVariableName) ?? string.Empty;should work.
There was a problem hiding this comment.
I will defer this as it broke recorded tests in an unexpected way. I'll look into it later
| @@ -140,7 +167,7 @@ protected static List<TestEntity> CreateCustomTableEntities(string partitionKeyV | |||
| GuidTypeProperty = new Guid($"0d391d16-97f1-4b9a-be68-4cc871f9{n:D4}"), | |||
| BinaryTypeProperty = new byte[] { 0x01, 0x02, 0x03, 0x04, 0x05 }, | |||
| Int64TypeProperty = long.Parse(number), | |||
| DoubleTypeProperty = (double)n, | |||
| DoubleTypeProperty = double.Parse($"{number}.0"), | |||
There was a problem hiding this comment.
Interesting change. What is the reason for it?
There was a problem hiding this comment.
It is to validate that serialization ignores the trailing .0. See #13552
|
Looks really nice! 👍 |
| // Both storage and cosmos can be inconsistent about requiring an Accept header, so add one here for all requests | ||
| if (message.Request.Headers.TryGetValue(TableConstants.HeaderNames.Content, out var contentType)) | ||
| { | ||
| switch (contentType) | ||
| { | ||
| case TableConstants.MimeType.ApplicationXml: | ||
| message.Request.Headers.SetValue(TableConstants.HeaderNames.Accept, TableConstants.MimeType.ApplicationXml); | ||
| break; | ||
| default: | ||
| message.Request.Headers.SetValue(TableConstants.HeaderNames.Accept, TableConstants.MimeType.ApplicationJson); | ||
| break; | ||
| } | ||
| } | ||
| else if (!message.Request.Uri.Query.Contains("comp=")) | ||
| { | ||
| // The default Accept header should be application/json, however all requests using the comp= query string are application/xml | ||
| // These requests don't always set a Content-Type header as a clue for the logic above. | ||
| message.Request.Headers.SetValue(TableConstants.HeaderNames.Accept, TableConstants.MimeType.ApplicationJson); | ||
| } |
There was a problem hiding this comment.
Could we make this change in the swagger? Or at least via README transform? This feels like the sort of thing we could fix at compile time instead of adding extra processing to every request.
There was a problem hiding this comment.
Agreed, I’m tracking this in the issue mentioned in the comment at the top of the class. Beyond the need for the header, the two services are inconsistent in their responses with or without this header.
3 participants
No description provided.