-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Bootstrap test resources when running tests #23652
Changes from all commits
c22c147
4b053ae
ed51d74
6641f07
7bc5784
767cb14
480aa9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Copyright (c) Microsoft Corporation. All rights reserved. | ||
# Licensed under the MIT License. | ||
|
||
param( | ||
[string] $ServiceDirectory | ||
) | ||
$run = Read-Host "The resources needed to run the live tests could not be located.`nWould you like to run the resource creation script? [y/n]" | ||
if ($run -eq 'y'){ | ||
& "$PSScriptRoot\..\common\TestResources\New-TestResources.ps1" $ServiceDirectory | ||
|
||
Read-Host "Press enter to close this window and resume your test run." | ||
} | ||
JoshLove-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,10 @@ | |
using System.Threading.Tasks; | ||
using Azure.Identity; | ||
using System.ComponentModel; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using System.Runtime.InteropServices; | ||
using System.Threading; | ||
using NUnit.Framework; | ||
|
||
namespace Azure.Core.TestFramework | ||
|
@@ -24,14 +27,22 @@ public abstract class TestEnvironment | |
[EditorBrowsableAttribute(EditorBrowsableState.Never)] | ||
public static string RepositoryRoot { get; } | ||
|
||
private static readonly Dictionary<Type, Task> s_environmentStateCache = new Dictionary<Type, Task>(); | ||
private static readonly Dictionary<Type, Task> s_environmentStateCache = new(); | ||
|
||
private readonly string _prefix; | ||
|
||
private TokenCredential _credential; | ||
private TestRecording _recording; | ||
private readonly string _serviceName; | ||
|
||
private readonly Dictionary<string, string> _environmentFile = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
private Dictionary<string, string> _environmentFile; | ||
private readonly string _serviceSdkDirectory; | ||
|
||
private static readonly HashSet<Type> s_bootstrappingAttemptedTypes = new(); | ||
private static readonly object s_syncLock = new(); | ||
private static readonly bool s_isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure how this builds then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we usually don't build tests for those frameworks...but Key Vault because there are differences we need to test for. |
||
private Exception _bootstrappingException; | ||
private readonly Type _type; | ||
|
||
protected TestEnvironment() | ||
{ | ||
|
@@ -42,28 +53,35 @@ protected TestEnvironment() | |
|
||
var testProject = GetSourcePath(GetType().Assembly); | ||
var sdkDirectory = Path.GetFullPath(Path.Combine(RepositoryRoot, "sdk")); | ||
var serviceName = Path.GetFullPath(testProject) | ||
_serviceName = Path.GetFullPath(testProject) | ||
JoshLove-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.Substring(sdkDirectory.Length) | ||
.Trim(Path.DirectorySeparatorChar) | ||
.Split(Path.DirectorySeparatorChar).FirstOrDefault(); | ||
|
||
if (string.IsNullOrWhiteSpace(serviceName)) | ||
if (string.IsNullOrWhiteSpace(_serviceName)) | ||
{ | ||
throw new InvalidOperationException($"Unable to determine the service name from test project path {testProject}"); | ||
} | ||
|
||
var serviceSdkDirectory = Path.Combine(sdkDirectory, serviceName); | ||
_serviceSdkDirectory = Path.Combine(sdkDirectory, _serviceName); | ||
if (!Directory.Exists(sdkDirectory)) | ||
{ | ||
throw new InvalidOperationException($"SDK directory {serviceSdkDirectory} not found"); | ||
throw new InvalidOperationException($"SDK directory {_serviceSdkDirectory} not found"); | ||
} | ||
|
||
_prefix = serviceName.ToUpperInvariant() + "_"; | ||
_prefix = _serviceName.ToUpperInvariant() + "_"; | ||
_type = GetType(); | ||
|
||
ParseEnvironmentFile(); | ||
} | ||
|
||
private void ParseEnvironmentFile() | ||
{ | ||
_environmentFile = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
var testEnvironmentFiles = new[] | ||
{ | ||
Path.Combine(serviceSdkDirectory, "test-resources.bicep.env"), | ||
Path.Combine(serviceSdkDirectory, "test-resources.json.env") | ||
Path.Combine(_serviceSdkDirectory, "test-resources.bicep.env"), | ||
Path.Combine(_serviceSdkDirectory, "test-resources.json.env") | ||
}; | ||
|
||
foreach (var testEnvironmentFile in testEnvironmentFiles) | ||
|
@@ -216,10 +234,10 @@ public async ValueTask WaitForEnvironmentAsync() | |
Task task; | ||
lock (s_environmentStateCache) | ||
{ | ||
if (!s_environmentStateCache.TryGetValue(GetType(), out task)) | ||
JoshLove-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!s_environmentStateCache.TryGetValue(_type, out task)) | ||
{ | ||
task = WaitForEnvironmentInternalAsync(); | ||
s_environmentStateCache[GetType()] = task; | ||
s_environmentStateCache[_type] = task; | ||
} | ||
} | ||
await task; | ||
|
@@ -304,6 +322,11 @@ protected string GetRecordedVariable(string name) | |
protected string GetRecordedVariable(string name, Action<RecordedVariableOptions> options) | ||
{ | ||
var value = GetRecordedOptionalVariable(name, options); | ||
if (value == null) | ||
{ | ||
BootStrapTestResources(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need these in Get* methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How else would we know if bootstrapping is required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check that RG exists. Would handle expiring resources as well as non-existant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That doesn't work - you could have an existent RG and still not have the resources. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can have an existing variable and no resource as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The concern that I have is the experience for external contributors. If I'm trying to get myself running and I encounter this because I'm on Linux or am using the documented/supported approach of environment variables on Windows, the experience isn't awesome. Will I bother to let you know or will I just get frustrated and give up without a word? I'd prefer that we optimize for not prompting unless we can prove that the environment isn't suitable to run, rather than optimizing for the common use case of the SDK team. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prompt will only happen on Windows. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, but I think the overall concern stands. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, if you are using env vars you may find the behavior annoying if we switch to check for the existence of an env file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the env vars aren't detected (or the file not found - normal behavior now) why not just point them at the instructions on how to run New-TestResources? Adding extra time to check all the time for resources existing will slow tests down even further and some of them cost by the hour. You also can't discern much from incremental deployments. Some resources might support it, while others don't because of the configuration they are in - like resources configured for Customer-Managed Keys (CMK) like sdk/search. You can't redeploy resources in that state incrementally - it messes everything up. |
||
value = GetRecordedOptionalVariable(name, options); | ||
} | ||
EnsureValue(name, value); | ||
return value; | ||
} | ||
|
@@ -348,6 +371,11 @@ protected string GetOptionalVariable(string name) | |
protected string GetVariable(string name) | ||
{ | ||
var value = GetOptionalVariable(name); | ||
if (value == null) | ||
{ | ||
BootStrapTestResources(); | ||
value = GetOptionalVariable(name); | ||
} | ||
EnsureValue(name, value); | ||
return value; | ||
} | ||
|
@@ -356,10 +384,18 @@ private void EnsureValue(string name, string value) | |
{ | ||
if (value == null) | ||
{ | ||
var prefixedName = _prefix + name; | ||
throw new InvalidOperationException( | ||
$"Unable to find environment variable {prefixedName} or {name} required by test." + Environment.NewLine + | ||
"Make sure the test environment was initialized using eng/common/TestResources/New-TestResources.ps1 script."); | ||
string prefixedName = _prefix + name; | ||
string message = $"Unable to find environment variable {prefixedName} or {name} required by test." + Environment.NewLine + | ||
JoshLove-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"Make sure the test environment was initialized using the eng/common/TestResources/New-TestResources.ps1 script."; | ||
JoshLove-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (_bootstrappingException != null) | ||
{ | ||
message += Environment.NewLine + "Resource creation failed during the test run. Make sure PowerShell version 6 or higher is installed."; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why PowerShell 6? Seems like tight coupling with something else, much like checking if this is Windows. If using env vars, why wouldn't other platforms work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It says at least powershell 6. This is the same messaging we use in the contributing.md file. |
||
throw new InvalidOperationException( | ||
message, | ||
_bootstrappingException); | ||
} | ||
|
||
throw new InvalidOperationException(message); | ||
} | ||
} | ||
|
||
|
@@ -459,5 +495,54 @@ internal static bool GlobalDisableAutoRecording | |
return disableAutoRecording || GlobalIsRunningInCI; | ||
} | ||
} | ||
|
||
private void BootStrapTestResources() | ||
{ | ||
lock (s_syncLock) | ||
{ | ||
try | ||
{ | ||
if (!s_isWindows || | ||
s_bootstrappingAttemptedTypes.Contains(_type) || | ||
Mode == RecordedTestMode.Playback || | ||
GlobalIsRunningInCI) | ||
Comment on lines
+506
to
+508
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd reorder these with faster checks first - basically, reverse these last three. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GlobalIsRunningInCI does an environment variable lookup whenever invoked. |
||
{ | ||
return; | ||
} | ||
|
||
string path = Path.Combine( | ||
RepositoryRoot, | ||
"eng", | ||
"scripts", | ||
$"New-TestResources-Bootstrapper.ps1 {_serviceName}"); | ||
|
||
var processInfo = new ProcessStartInfo( | ||
@"pwsh.exe", | ||
path) | ||
{ | ||
UseShellExecute = true | ||
}; | ||
Process process = null; | ||
try | ||
{ | ||
process = Process.Start(processInfo); | ||
} | ||
catch (Exception ex) | ||
{ | ||
_bootstrappingException = ex; | ||
} | ||
|
||
if (process != null) | ||
{ | ||
process.WaitForExit(); | ||
ParseEnvironmentFile(); | ||
} | ||
} | ||
finally | ||
{ | ||
s_bootstrappingAttemptedTypes.Add(_type); | ||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not in Azure/azure-sdk-tools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only going to be used by the .NET test suite.