Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request centralizes the verification logic by replacing explicit calls to Verifier.Verify(...).UseHelixAwareDirectory(...) with a unified Verify(...) call that implicitly handles HELIX constraints.
- Updated test methods across various Azure test files to align with the new central Verify convention.
- Removed redundant chaining of UseHelixAwareDirectory to avoid duplication of HELIX directory logic.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Azure.Tests/AzureWebPubSubExtensionsTests.cs | Removed explicit HELIX directory usage in Verify calls. |
| tests/Aspire.Hosting.Azure.Tests/AzureUserAssignedIdentityTests.cs | Replaced Verifier.Verify with centralized Verify for identity tests. |
| tests/Aspire.Hosting.Azure.Tests/AzureStorageExtensionsTests.cs | Updated Verify call by eliminating UseHelixAwareDirectory. |
| tests/Aspire.Hosting.Azure.Tests/AzureSqlExtensionsTests.cs | Unified Verify method call across SQL test scenarios. |
| tests/Aspire.Hosting.Azure.Tests/AzureSignalRExtensionsTests.cs | Removed HELIX-specific chaining from SignalR tests. |
| tests/Aspire.Hosting.Azure.Tests/AzureServiceBusExtensionsTests.cs | Similar update with Verify call for ServiceBus tests. |
| tests/Aspire.Hosting.Azure.Tests/AzureResourcePreparerTests.cs | Transitioned to centralized Verify without HELIX directory usage. |
| tests/Aspire.Hosting.Azure.Tests/AzureResourceOptionsTests.cs | Updated Verify call, removing explicit HELIX directory chaining. |
| tests/Aspire.Hosting.Azure.Tests/AzureRedisExtensionsTests.cs | Consolidated Verify call to central Verify. |
| tests/Aspire.Hosting.Azure.Tests/AzureProvisioningResourceExtensionsTests.cs | Standardized Verify call by removing UseHelixAwareDirectory. |
| tests/Aspire.Hosting.Azure.Tests/AzurePostgresExtensionsTests.cs | Adjusted Verify usage to the new centralized convention. |
| tests/Aspire.Hosting.Azure.Tests/AzureKeyVaultTests.cs | Removed HELIX directory chaining in favor of central Verify logic. |
| tests/Aspire.Hosting.Azure.Tests/AzureFunctionsTests.cs | Updated Verify calls in functions tests with the central method. |
| tests/Aspire.Hosting.Azure.Tests/AzureEventHubsExtensionsTests.cs | Removed explicit HELIX directory usage in Verify chain. |
| tests/Aspire.Hosting.Azure.Tests/AzureEnvironmentResourceTests.cs | Transitioned to centralized Verify without HELIX configuration. |
| tests/Aspire.Hosting.Azure.Tests/AzureCosmosDBExtensionsTests.cs | Unified Verify usage by removing HELIX chaining. |
| tests/Aspire.Hosting.Azure.Tests/AzureContainerRegistryTests.cs | Updated Verify call to align with the new convention. |
| tests/Aspire.Hosting.Azure.Tests/AzureContainerAppsTests.cs | Replaced multiple instances of UseHelixAwareDirectory with Verify. |
| tests/Aspire.Hosting.Azure.Tests/AzureBicepResourceTests.cs | Replaced explicit HELIX directory calls with central Verify. |
| tests/Aspire.Hosting.Azure.Tests/AzureAppServiceTests.cs | Updated Verify calls to remove direct HELIX directory specification. |
Comments suppressed due to low confidence (1)
tests/Aspire.Hosting.Azure.Tests/AzureWebPubSubExtensionsTests.cs:61
- Ensure that the new Verify method centrally applies the HELIX directory logic previously provided by UseHelixAwareDirectory, so that snapshot output remains in the intended location.
await Verifier.Verify(manifest.BicepText, extension: "bicep")
|
|
||
| await Verifier.Verify(bicep, extension: "bicep") | ||
| .UseHelixAwareDirectory(); | ||
| await Verify(bicep, extension: "bicep"); |
There was a problem hiding this comment.
Consider adding a documentation comment in the Verify method definition to clarify that it now implicitly handles the HELIX constraints, removing the need for an explicit UseHelixAwareDirectory call.
Member
|
Nice! Merge these so we have one module initializer https://github.com/dotnet/aspire/blob/main/tests/Shared/TestModuleInitializer.cs |
davidfowl
approved these changes
May 17, 2025
Member
|
Watch the main build |
Member
Author
|
|
RussKie
reviewed
May 18, 2025
| /// <summary> | ||
| /// Sets the directory for the Verify call in a way it's also compatible with Helix. | ||
| /// </summary> | ||
| public static SettingsTask UseHelixAwareDirectory(this SettingsTask settings, string directory = "Snapshots") |
Contributor
There was a problem hiding this comment.
Is this still used somewhere?
Member
Author
There was a problem hiding this comment.
It's not.
Didn't want to remove it yet to keep the knowledge of how it's done.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found a way to centrally apply the HELIX contraints on
Verifycalls.