Add integration testing #634
Conversation
There was a problem hiding this comment.
PR Overview
This PR introduces the first draft for integration testing using Copilot. The changes in this PR primarily involve reordering the using directives in two configuration files.
- Updated using statements ordering in ConfigurationSettingPageExtensions.cs.
- Updated using statements ordering in IConfigurationSettingPageIterator.cs.
Reviewed Changes
| File | Description |
|---|---|
| src/Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationSettingPageExtensions.cs | Reordered using directives |
| src/Microsoft.Extensions.Configuration.AzureAppConfiguration/IConfigurationSettingPageIterator.cs | Reordered using directives |
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
| } | ||
| } | ||
| } | ||
| catch (Exception ex) |
| { | ||
| string localSettingsPath = Path.Combine(AppContext.BaseDirectory, LocalSettingsFile); | ||
|
|
||
| if (File.Exists(localSettingsPath)) |
There was a problem hiding this comment.
Can the tests run without the file?
| { | ||
| /// <summary> | ||
| /// Integration tests for Azure App Configuration that connect to a real service. | ||
| /// Creates a temporary App Configuration store for testing and deletes it after the tests are complete. |
There was a problem hiding this comment.
How are stale stores cleaned up?
| new ConfigurationSetting(SentinelKey, "Initial"), | ||
| ConfigurationModelFactory.ConfigurationSetting( | ||
| FeatureFlagKey, | ||
| @"{""id"":""" + TestKeyPrefix + @"Feature"",""description"":""Test feature"",""enabled"":false}", |
| private const string LocationEnvVar = "AZURE_LOCATION"; | ||
| private const string CreateResourceGroupEnvVar = "AZURE_CREATE_RESOURCE_GROUP"; | ||
| private const string DefaultLocation = "eastus"; | ||
| private const string LocalSettingsFile = "local.settings.json"; |
There was a problem hiding this comment.
We typically use appsettings.json or appsettings.test.json
| // Azure Resource Management constants | ||
| private const string ResourceGroupEnvVar = "AZURE_APPCONFIG_RESOURCE_GROUP"; | ||
| private const string SubscriptionIdEnvVar = "AZURE_SUBSCRIPTION_ID"; | ||
| private const string LocationEnvVar = "AZURE_LOCATION"; |
There was a problem hiding this comment.
Is there really a desire to run from env variable?
| private const string SubscriptionIdEnvVar = "AZURE_SUBSCRIPTION_ID"; | ||
| private const string LocationEnvVar = "AZURE_LOCATION"; | ||
| private const string CreateResourceGroupEnvVar = "AZURE_CREATE_RESOURCE_GROUP"; | ||
| private const string DefaultLocation = "eastus"; |
There was a problem hiding this comment.
Let's pick a low traffic region.
| /// <summary> | ||
| /// Cleans up the temporary App Configuration store after tests are complete. | ||
| /// </summary> | ||
| public async Task DisposeAsync() |
There was a problem hiding this comment.
I think we should have Cleanup method that we call on intialization and tear down. That way we don't have stale stores.
| await _appConfigStore.DeleteAsync(WaitUntil.Completed); | ||
| } | ||
|
|
||
| if (_shouldDeleteResourceGroup && _resourceGroup != null) |
There was a problem hiding this comment.
Do you think we'll ever want to delete the resource group?
| .Build(); | ||
|
|
||
| // Verify initial values | ||
| Assert.Equal("InitialValue1", config[$"{keyPrefix}:Setting1"]); |
There was a problem hiding this comment.
I think you should have one or multiple pre-cache expiration calls to ensure that the keys aren't refreshed until the cache expiration is up. That way cache expiration both ways can be tested here.
| [Fact] | ||
| public void LoadConfiguration_RetrievesValuesFromAppConfiguration() | ||
| { | ||
| Skip.If(_skipTests, _skipReason); |
There was a problem hiding this comment.
I don't really understand why we'd want skip. If an exception occurs during initialization, just let it bubble and blow up.
| await Task.Delay(TimeSpan.FromSeconds(2)); | ||
|
|
||
| // Act | ||
| var result = await refresher.TryRefreshAsync(); |
There was a problem hiding this comment.
I would use RefreshAsync instead of Try.
| // Only refresh Setting1 when sentinel changes | ||
| options.ConfigureRefresh(refresh => | ||
| { | ||
| refresh.Register(sentinelKey, $"{keyPrefix}:Setting1", refreshAll: false) |
There was a problem hiding this comment.
Is this registering a key with a label $"{keyPrefix}:Setting1" ? That looks wrong.
| /// <summary> | ||
| /// Setup test-specific keys and settings | ||
| /// </summary> | ||
| private async Task<(string keyPrefix, string sentinelKey, string featureFlagKey)> SetupTestKeys(string testName) |
| } | ||
|
|
||
| [Fact] | ||
| public async Task RegisterAll_RefreshesAllKeys_WhenSentinelChanged() |
There was a problem hiding this comment.
sentinel and refresh all are two completely different paths. I don't see a reason to mix them.
First draft of integration testing using copilot