Enabled Sample Testing by making samples Inherit from DocumentTranslationLiveTestBase #23072
Conversation
…s Error Use Cases.
…error as the third parameter.
…lar to the error status generator.
…anslationLiveTestBase
...lation/Azure.AI.Translation.Document/tests/Infrastructure/DocumentTranslationLiveTestBase.cs
Show resolved
Hide resolved
|
|
||
| public Uri CreateTargetContainer(List<TestDocument> documents = default) | ||
| { | ||
| Recording.DisableIdReuse(); |
There was a problem hiding this comment.
I wonder if a good way to avoid having the same logic in so many functions is to create 2 functions (one sync and one async) with logic:1. get a container name, create the container, upload the document, return the container,
and then source, target, and glossary functions can use call those functions.
What do you think?
There was a problem hiding this comment.
Yes! Sounds great. Will implement this first thing tomorrow.
There was a problem hiding this comment.
I somehow got a notification from you asking about the permissions. That is a good point.
We can make them a parameter too
There was a problem hiding this comment.
Oh yea, I initially had a comment about that but I replaced it thinking that I can just return the containerClient like you mentioned. Then the caller function can generate the Sas with whichever permissions it needs. Not sure which one is more elegant.
There was a problem hiding this comment.
I think both have pros and cons. whichever should be ok
| public partial class DocumentTranslationSamples : DocumentTranslationLiveTestBase | ||
| { | ||
| public DocumentTranslationSamples(bool async) | ||
| : base(async, RecordedTestMode.Live) |
There was a problem hiding this comment.
we shouldn't need to set the RecordedTestMode.Live as the CI will set it for us.
You should be able to only do : base(async)
| /// Samples that are used in the associated README.md file. | ||
| /// </summary> | ||
| public partial class SampleSnippets : SamplesBase<DocumentTranslationTestEnvironment> | ||
| public partial class SampleSnippets : DocumentTranslationLiveTestBase |
There was a problem hiding this comment.
looking at the sample code we have here, we don't actually need for this class to inherit from DocumentTranslationLiveTestBase. We can just leave it as it
|
|
||
| [Test] | ||
| [Ignore("Samples not working yet")] | ||
| [SyncOnly] |
There was a problem hiding this comment.
This shouldn't be added. The sample is just showing inputs and there is no actual call to client.StartTranslation therefore, we don't need to actually call Uri sourceSasUri = CreateSourceContainer(oneTestDocuments);
| Uri source1SasUri = new Uri("<source1 SAS URI>"); | ||
| Uri source2SasUri = new Uri("<source2 SAS URI>"); | ||
| #else | ||
| Uri source1SasUri = CreateSourceContainer(oneTestDocuments); |
There was a problem hiding this comment.
see my comment above about how this is not needed for this sample
There was a problem hiding this comment.
Yea it makes sense that we don't actually need to create containers, but it seems like there is some form of format checking that takes place. A System.UriFormatException : Invalid URI exception is thrown if the sample input is used. I thought that creating an actual container would suffice, but you're right that it is unnecessary.
I could however just use any other sample uri, like the url for the github repo for example. Or is there a standard one that I should use?
There was a problem hiding this comment.
I think once you remove the inheritance from DocumentTranslationLiveTestBase back to SamplesBase<DocumentTranslationTestEnvironment> you should be ok
There was a problem hiding this comment.
Unfortunately the format exception is still thrown. I believe it is thrown by the Uri constructor as it does its own checks on the input string format.
|
/azp run net - translation - tests |
|
Commenter does not have sufficient privileges for PR 23072 in repo Azure/azure-sdk-for-net |
|
/azp run net - translation - tests |
|
Pull request contains merge conflicts. |
…r "CreateTargetContainerWithClientAsync"
...lation/Azure.AI.Translation.Document/tests/Infrastructure/DocumentTranslationLiveTestBase.cs
Outdated
Show resolved
Hide resolved
sdk/translation/Azure.AI.Translation.Document/tests/samples/Sample_PollIndividualDocuments.cs
Outdated
Show resolved
Hide resolved
…samples Fixed minor bug that resulted from the merger.
|
/azp run net - translation - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…age variables in "StartTranslationWithAzureBlob" sample.
|
/azp run net - translation - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
After reviewing the KeyVault and Cognitive Search SDKs, I found that both Search and Keyvault.Admin were set up such that the samples classes inherit from their TestBases that were [RecordedTestBases].
In Search, each test was set to be either [SyncOnly] or [AsyncOnly]. See here
As such this is the approach I used here.