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.

Sign up for GitHub

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

Jump to bottom

[Storage][Core] Wait for environment to become eventually consistent. #20574

Merged
merged 9 commits into from
Apr 21, 2021
Merged

[Storage][Core] Wait for environment to become eventually consistent. #20574

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc7de1c
draft.
kasobol-msft Apr 21, 2021
d218694
tweaks + tests.
kasobol-msft Apr 21, 2021
a14e551
pr feedback.
kasobol-msft Apr 21, 2021
97e2a6e
PR feedback.
kasobol-msft Apr 21, 2021
dd244f4
rename.
kasobol-msft Apr 21, 2021
854d531
rename.
kasobol-msft Apr 21, 2021
6f5fd5b
valuetask.
kasobol-msft Apr 21, 2021
2852c2d
readme.
kasobol-msft Apr 21, 2021
b082658
pr feedback.
kasobol-msft Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions sdk/core/Azure.Core.TestFramework/src/RecordedTestBaseOfTEnvironment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Threading.Tasks;
using NUnit.Framework;

namespace Azure.Core.TestFramework
{
#pragma warning disable SA1649 // File name should match first type name
Expand All @@ -23,5 +27,11 @@ public override void StartTestRecording()
}

public TEnvironment TestEnvironment { get; }

[OneTimeSetUp]
public async Task WaitForEnvironment()
pakrym marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this ValueTask since it will usually complete synchronously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If somebody decides they need "sampling" they'll most likely end up calling API like this:
https://github.com/kasobol-msft/azure-sdk-for-net/blob/dd244f41ee9c999dc09cb42cbd72930f1320770d/sdk/storage/Azure.Storage.Blobs/tests/BlobTestEnvironment.cs#L12-L30
So, this task here would chain to async api call worst case.
Could you elaborate more how would you use ValueTask in this logic?

Copy link
Member

@JoshLove-msft JoshLove-msft Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, if the method isn't overriden then it would complete synchronously. But I suppose since we are returning Task<bool> the runtime is able to use cached instances for these so it probably doesn't provide much benefit. Also, we'd have to call AsTask anyway when putting them into the dictionary, so I think using Task makes sense.

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/#task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good read. How does it look now?
One place I had to retain task is the cache as we shouldn't be await on ValueTasks multiple times (per that article).

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that is what I was thinking when I mentioned needing to call AsTask, but I think how you have it is even better.

Copy link
Member

Choose a reason for hiding this comment

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

Being that we still have to allocate a task, I don't think there is any practical difference between using ValueTask<T> and Task<T> here, but ValueTask still seems to convey the intent of the API better. /cc @pakrym to keep me honest here

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say task vs valuetask doesn't matter in this situation at all. We have so many way larger inefficiencies in the test framework.

But ValueTask<T> and Task<T> difference matters for sync completion mostly. ValueTask<T> won't allocate when completed synchronously. But Task<bool> return value are cached(by the runtime) so it won't allocate as well

{
await TestEnvironment.WaitForEnvironment();
}
}
}
65 changes: 65 additions & 0 deletions sdk/core/Azure.Core.TestFramework/src/TestEnvironment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.ComponentModel;
using System.Linq;
using NUnit.Framework;
using System.Collections.Concurrent;

namespace Azure.Core.TestFramework
{
Expand All @@ -25,6 +26,8 @@ public abstract class TestEnvironment
[EditorBrowsableAttribute(EditorBrowsableState.Never)]
public static string RepositoryRoot { get; }

private static readonly IDictionary<Type, EnvironmentReadyState> EnvironmentStateCache = new ConcurrentDictionary<Type, EnvironmentReadyState>();

private readonly string _prefix;

private TokenCredential _credential;
Expand Down Expand Up @@ -175,6 +178,68 @@ public TokenCredential Credential
}
}

/// <summary>
/// Returns whether environment is ready to use. Should be overriden to provide service specific sampling scenario.
/// The test framework will wait until this returns true before starting tests.
/// Use this place to hook up logic that polls if eventual consistency has happened.
///
/// Return true if environment is ready to use.
/// Return false if environment is not ready to use and framework should wait.
/// Throw if you want to fail the run fast.
/// </summary>
/// <returns>Whether environment is ready to use.</returns>
public virtual Task<bool> IsEnvironmentReady()
Copy link
Contributor

Choose a reason for hiding this comment

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

protected

{
return Task.FromResult(true);
}

/// <summary>
/// Waits until environment becomes ready to use. See <see cref="IsEnvironmentReady"/> to define sampling scenario.
/// </summary>
/// <returns>A task.</returns>
public async Task WaitForEnvironment()
Copy link
Member

Choose a reason for hiding this comment

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

Consider suffixing with Async.

{
Copy link
Contributor

Choose a reason for hiding this comment

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

We might have a problem if multiple classes call this method in parallel. Consider having an IDictionary<Type, Task> where we initialize the task immediately and then await on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also have a nice side-effect of caching the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup and it got rid of the enum.

Copy link
Member

@JoshLove-msft JoshLove-msft Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we need per-class invocations of WaitForEnvironmentInternal? The logic can't vary per test class, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We call into virtual method where service team provides sampling scenario. I don't think we can do this from static context.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, this is per Environment type.

if (EnvironmentStateCache.TryGetValue(GetType(), out var environmentState))
{
switch (environmentState)
{
case EnvironmentReadyState.Ready:
return;
case EnvironmentReadyState.Failed:
throw new InvalidOperationException("The environment has not become ready, check your TestEnvironment.IsEnvironmentReady scenario.");
}
}

try
pakrym marked this conversation as resolved.
Show resolved Hide resolved
{
int numberOfTries = 60;
TimeSpan delay = TimeSpan.FromSeconds(10);
for (int i = 0; i < numberOfTries; i++)
{
var isReady = await IsEnvironmentReady();
if (isReady)
{
EnvironmentStateCache[GetType()] = EnvironmentReadyState.Ready;
return;
}
await Task.Delay(delay);
}
}
catch (Exception e)
{
EnvironmentStateCache[GetType()] = EnvironmentReadyState.Failed;
throw new InvalidOperationException("TestEnvironment.IsEnvironmentReady threw, check your TestEnvironment.IsEnvironmentReady scenario.", e);
}

EnvironmentStateCache[GetType()] = EnvironmentReadyState.Failed;
throw new InvalidOperationException("The environment has not become ready, check your TestEnvironment.IsEnvironmentReady scenario.");
}

private enum EnvironmentReadyState
{
Ready, Failed
}

/// <summary>
/// Returns and records an environment variable value when running live or recorded value during playback.
/// </summary>
Expand Down
62 changes: 62 additions & 0 deletions sdk/core/Azure.Core/tests/TestEnvironmentTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
using System.Security.Cryptography;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Azure.Core.Serialization;
using Azure.Core.TestFramework;
using Moq;
using NUnit.Framework;

namespace Azure.Core.Tests
Expand Down Expand Up @@ -223,5 +225,65 @@ private class MockTestEnvironment : TestEnvironment
public string MissingOptionalSecret => GetRecordedOptionalVariable("MissingOptionalSecret", option => option.IsSecret("INVALID"));
public string ConnectionStringWithSecret => GetRecordedVariable("ConnectionStringWithSecret", option => option.HasSecretConnectionStringParameter("key"));
}

public class WaitForEnvironmentTestClassOne : RecordedTestBase<WaitForEnvironmentTestEnvironmentOne>
{
public WaitForEnvironmentTestClassOne(bool isAsync) : base(isAsync)
{
}

[Test]
public void ShouldCacheStateCorrectly()
{
Assert.AreEqual(2, WaitForEnvironmentTestEnvironmentOne.InvocationCount);
}
}

public class WaitForEnvironmentTestClassTwo : RecordedTestBase<WaitForEnvironmentTestEnvironmentTwo>
{
public WaitForEnvironmentTestClassTwo(bool isAsync) : base(isAsync)
{
}

[Test]
public void ShouldCacheStateCorrectly()
{
Assert.AreEqual(2, WaitForEnvironmentTestEnvironmentTwo.InvocationCount);
}
}

// This one uses same env as WaitForEnvironmentTestClassTwo to prove value is cached.
public class WaitForEnvironmentTestClassThree : RecordedTestBase<WaitForEnvironmentTestEnvironmentTwo>
{
public WaitForEnvironmentTestClassThree(bool isAsync) : base(isAsync)
{
}

[Test]
public void ShouldCacheStateCorrectly()
{
Assert.AreEqual(2, WaitForEnvironmentTestEnvironmentTwo.InvocationCount);
}
}

public class WaitForEnvironmentTestEnvironmentOne : TestEnvironment
{
public static int InvocationCount { get; private set; }

public override Task<bool> IsEnvironmentReady()
{
return Task.FromResult(InvocationCount++ < 1 ? false : true);
}
}

public class WaitForEnvironmentTestEnvironmentTwo : TestEnvironment
{
public static int InvocationCount { get; private set; }

public override Task<bool> IsEnvironmentReady()
{
return Task.FromResult(InvocationCount++ < 1 ? false : true);
}
}
}
}
1 change: 1 addition & 0 deletions sdk/storage/Azure.Storage.Blobs.Batch/tests/Azure.Storage.Blobs.Batch.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<ItemGroup>
<Compile Include="$(AzureCoreSharedSources)ArrayBufferWriter.cs" Link="Shared\Core\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(MSBuildThisFileDirectory)..\..\Azure.Storage.Blobs\tests\BlobTestBase.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(MSBuildThisFileDirectory)..\..\Azure.Storage.Blobs\tests\BlobTestEnvironment.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureStorageSharedSources)StorageConnectionString.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureStorageSharedSources)SharedAccessSignatureCredentials.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
<Compile Include="$(AzureStorageSharedSources)UriExtensions.cs" Link="Shared\%(RecursiveDir)\%(Filename)%(Extension)" />
Expand Down
2 changes: 1 addition & 1 deletion sdk/storage/Azure.Storage.Blobs.ChangeFeed/tests/ChangeFeedTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Azure.Storage.Blobs.ChangeFeed.Tests
StorageVersionExtensions.MaxVersion,
RecordingServiceVersion = StorageVersionExtensions.MaxVersion,
LiveServiceVersions = new object[] { StorageVersionExtensions.LatestVersion })]
public class ChangeFeedTestBase : StorageTestBase
public class ChangeFeedTestBase : StorageTestBase<StorageTestEnvironment>
{
protected readonly BlobClientOptions.ServiceVersion _serviceVersion;

Expand Down
38 changes: 19 additions & 19 deletions sdk/storage/Azure.Storage.Blobs/tests/BlobBaseClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4472,7 +4472,7 @@ public async Task GetPropertiesAsync_LastAccessed()
[RecordedTest]
public async Task SetHttpHeadersAsync()
{
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
await using DisposingContainer test = await GetTestContainerAsync();
// Arrange
BlobBaseClient blob = await GetNewBlobClient(test.Container);
Expand Down Expand Up @@ -4501,7 +4501,7 @@ await blob.SetHttpHeadersAsync(new BlobHttpHeaders
[RecordedTest]
public async Task SetHttpHeadersAsync_MultipleHeaders()
{
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
await using DisposingContainer test = await GetTestContainerAsync();
// Arrange
BlobBaseClient blob = await GetNewBlobClient(test.Container);
Expand Down Expand Up @@ -5914,7 +5914,7 @@ await TestHelper.AssertExpectedExceptionAsync<RequestFailedException>(
public async Task SetTierAsync_Version()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
await using DisposingContainer test = await GetTestContainerAsync();
var data = GetRandomBuffer(Constants.KB);
BlockBlobClient blob = InstrumentClient(test.Container.GetBlockBlobClient(GetNewBlobName()));
Expand Down Expand Up @@ -6426,7 +6426,7 @@ await TestHelper.AssertExpectedExceptionAsync<RequestFailedException>(
public void CanGenerateSas_ClientConstructors()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);
var blobSecondaryEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account + "-secondary");
var storageConnectionString = new StorageConnectionString(constants.Sas.SharedKeyCredential, blobStorageUri: (blobEndpoint, blobSecondaryEndpoint));
Expand Down Expand Up @@ -6473,7 +6473,7 @@ public void CanGenerateSas_ClientConstructors()
public void CanGenerateSas_GetParentBlobContainerClient()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);
var blobSecondaryEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account + "-secondary");
var storageConnectionString = new StorageConnectionString(constants.Sas.SharedKeyCredential, blobStorageUri: (blobEndpoint, blobSecondaryEndpoint));
Expand Down Expand Up @@ -6525,7 +6525,7 @@ public void CanGenerateSas_GetParentBlobContainerClient()
public void CanGenerateSas_WithSnapshot_True()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);
var blobSecondaryEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account + "-secondary");
var storageConnectionString = new StorageConnectionString(constants.Sas.SharedKeyCredential, blobStorageUri: (blobEndpoint, blobSecondaryEndpoint));
Expand All @@ -6550,7 +6550,7 @@ public void CanGenerateSas_WithSnapshot_True()
public void CanGenerateSas_WithSnapshot_False()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);

// Create blob
Expand All @@ -6571,7 +6571,7 @@ public void CanGenerateSas_WithSnapshot_False()
public void CanGenerateSas_WithVersion_True()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);
var blobSecondaryEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account + "-secondary");
var storageConnectionString = new StorageConnectionString(constants.Sas.SharedKeyCredential, blobStorageUri: (blobEndpoint, blobSecondaryEndpoint));
Expand All @@ -6596,7 +6596,7 @@ public void CanGenerateSas_WithVersion_True()
public void CanGenerateSas_WithVersion_False()
{
// Arrange
var constants = new TestConstants(this);
var constants = TestConstants.Create(this);
var blobEndpoint = new Uri("https://127.0.0.1/" + constants.Sas.Account);

// Create blob
Expand Down Expand Up @@ -6634,7 +6634,7 @@ public void CanGenerateSas_Mockable()
public void GenerateSas_RequiredParameters()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string containerName = GetNewContainerName();
string blobName = GetNewBlobName();
Uri serviceUri = new Uri($"https://{constants.Sas.Account}.blob.core.windows.net");
Expand Down Expand Up @@ -6671,7 +6671,7 @@ public void GenerateSas_RequiredParameters()
[RecordedTest]
public void GenerateSas_Builder()
{
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string containerName = GetNewContainerName();
string blobName = GetNewBlobName();
Uri serviceUri = new Uri($"https://{constants.Sas.Account}.blob.core.windows.net");
Expand Down Expand Up @@ -6718,7 +6718,7 @@ public void GenerateSas_Builder()
public void GenerateSas_BuilderNullContainerName()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobName = GetNewBlobName();
string containerName = GetNewContainerName();
Uri serviceUri = new Uri($"https://{constants.Sas.Account}.blob.core.windows.net");
Expand Down Expand Up @@ -6763,7 +6763,7 @@ public void GenerateSas_BuilderNullContainerName()
public void GenerateSas_BuilderWrongContainerName()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobName = GetNewBlobName();
string containerName = GetNewContainerName();
BlobUriBuilder blobUriBuilder = new BlobUriBuilder(new Uri($"https://{constants.Sas.Account}.blob.core.windows.net"))
Expand Down Expand Up @@ -6795,7 +6795,7 @@ public void GenerateSas_BuilderWrongContainerName()
public void GenerateSas_BuilderNullBlobName()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobName = GetNewBlobName();
string containerName = GetNewContainerName();
Uri serviceUri = new Uri($"https://{constants.Sas.Account}.blob.core.windows.net");
Expand Down Expand Up @@ -6843,7 +6843,7 @@ public void GenerateSas_BuilderNullBlobName()
public void GenerateSas_BuilderWrongBlobName()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string containerName = GetNewContainerName();
BlobUriBuilder blobUriBuilder = new BlobUriBuilder(new Uri($"https://{constants.Sas.Account}.blob.core.windows.net"))
{
Expand Down Expand Up @@ -6873,7 +6873,7 @@ public void GenerateSas_BuilderWrongBlobName()
public void GenerateSas_BuilderNullSnapshot()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobName = GetNewBlobName();
string containerName = GetNewContainerName();
string snapshot = "2020-07-03T12:45:46.1234567Z";
Expand Down Expand Up @@ -6926,7 +6926,7 @@ public void GenerateSas_BuilderNullSnapshot()
public void GenerateSas_BuilderWrongSnapshot()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string snapshot = "2020-07-03T12:45:46.1234567Z";
string differentSnapshot = "2019-07-03T12:45:46.1234567Z";
string containerName = GetNewContainerName();
Expand Down Expand Up @@ -6962,7 +6962,7 @@ public void GenerateSas_BuilderWrongSnapshot()
public void GenerateSas_BuilderNullVersion()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobName = GetNewBlobName();
string containerName = GetNewContainerName();
string versionId = "2020-07-03T12:45:46.1234567Z";
Expand Down Expand Up @@ -7015,7 +7015,7 @@ public void GenerateSas_BuilderNullVersion()
public void GenerateSas_BuilderWrongVersion()
{
// Arrange
TestConstants constants = new TestConstants(this);
TestConstants constants = TestConstants.Create(this);
string blobVersionId = "2020-07-03T12:45:46.1234567Z";
string diffBlobVersionId = "2019-07-03T12:45:46.1234567Z";
string containerName = GetNewContainerName();
Expand Down
Loading