Robe/azure external test categorization#2040
Open
zentron wants to merge 12 commits into
Open
Conversation
Introduce a single ExternalCloudIntegration test category and apply it to every Azure test fixture that authenticates against or calls a real Azure service. Tests without the category are treated as unit/integration tests that need no external service, so CI can run them with no credentials and run the real-cloud fixtures as a separate smoke suite. The two AppService integration base classes are tagged so all derived fixtures inherit the category (NUnit categories are inherited). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Target discovery only touched Azure through a single Resource Graph query (GetResourcesByType). The surrounding tag-matching, slot and service-message logic is pure Calamari code, yet it was only covered by TargetDiscoveryBehaviourIntegrationTestFixture, which authenticated against and queried the real static resource group. Introduce IAzureWebAppDiscoverer over that one call and inject it into TargetDiscoveryBehaviour (registered in Program.cs). With the Azure call mockable, the five tag-matching scenarios move into the credential-free TargetDiscoveryBehaviourUnitTestFixture, and the live-Azure integration fixture is removed. Its remaining real-cloud value (that the Resource Graph query and auth still work) is replaced by a thin smoke test in a following commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dIntegration The real-cloud fixtures previously derived from two base classes (AppServiceIntegrationTest and AppServiceIntegrationTestWithStaticResources) that each independently authenticated and built an ArmClient. Replace them with a 3-level hierarchy whose only genuine difference is resource lifecycle: AzureAppServiceTestBase - authenticates + ArmClient, once |- AzureAppServiceWithProvisionedResourcesTestBase - creates/destroys its own RG |- AzureAppServiceWithStaticResourcesTestBase - reuses calamari-testing-static-rg NUnit runs the base [OneTimeSetUp] before the derived one, so the base authenticates and the child provisions or looks up its resources. Every fixture that talks to real Azure moves into an ExternalCloudIntegration/ folder so the credential-free vs live-Azure split is visible in the tree, and functionapp.1.0.0.zip moves with the only fixture that uses it (a csproj <Link> keeps it copying to the output root). The target-discovery smoke test that replaces the integration fixture removed in the previous commit is added here, now that the static-resource base exists. This commit is behaviour-preserving: fixtures are relocated and re-pointed at the new bases, but no test logic is removed - those changes follow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eploy Container deployment made several Azure calls inline: detecting the app service OS, then reading and writing site config and app settings. The only Calamari-owned logic around them is the Windows/Linux FxVersion branch, the registry/image app settings, and slot-vs-site targeting. Introduce IAzureAppServiceContainerConfigurer over those calls and inject it into AzureAppServiceContainerDeployBehaviour (threaded through AppDeployBehaviour and registered in Program.cs). With Azure mockable, the OS branch and targeting logic move into the credential-free AzureAppServiceContainerDeployBehaviourUnitTestFixture, covering both Windows and Linux. That makes the separate Windows cloud fixture redundant - the "Windows vs Linux" cloud split was only ever the OS branch - so it is removed. The remaining cloud fixture is a single Linux smoke test proving auth and the slot-vs-site ARM round-trip still work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Choosing the IPackageProvider from a package's file extension - which sets the Kudu upload endpoint and whether the deploy is synchronous or async - was an inline switch in AzureAppServiceZipDeployBehaviour, only covered through full cloud deploys. Move it into PackageProviderFactory and unit-test the routing directly: .zip/.nupkg/.war/.jar each map to the expected upload URL and sync/async mode, and an unsupported extension throws. Notably .war and .jar route to the same JavaPackageProvider, differing only by upload URL - so a real-cloud deploy of each adds no coverage. This sets up removing the WAR/JAR cloud tests next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WAR and JAR both route through JavaPackageProvider, differing only by the Kudu upload URL - which PackageProviderFactoryFixture now verifies without Azure. The cloud tests spun up a dedicated Tomcat/Java app service purely to assert the deploy succeeded, adding no coverage beyond that unit test while paying for a real provision-and-deploy. Remove both CanDeployWarPackage and the two CanDeployJarPackage tests, along with the appServicePlanResource fields that only existed to host the Java sites, and delete the now-orphaned sample.1.0.0.war and sample4-1.0.0.jar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CanDeployWebAppZip_WithAsyncDeploymentAndPolling, CanDeployNugetPackage_WithAsyncDeploymentAndPolling and CanDeployZip_ToLinuxFunctionApp_WithAsyncDeploymentAndPolling each differed from their plain counterparts only by an arrange block that read the enabled feature toggles and wrote the identical values back - a no-op. The async path is not toggle-gated; it is selected by packageProvider.SupportsAsynchronousDeployment (true for zip and nuget), so the baseline zip/nuget deploy tests already cover it. These three were behavioural duplicates, each paying for a second real cloud deploy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test did not deploy two targets in parallel: it opened the package file with a read share-lock and ran a single deployment, asserting only that the outcome was successful. It was really checking that Calamari's package staging tolerates a concurrently-held file handle - a generic Calamari.Common file-share concern, not an Azure App Service one - while paying for a full real-cloud deploy. Remove it along with its sole helper PrepareFunctionAppZipPackage and the FileShare alias. If the file-locking behaviour is worth covering, it belongs in Calamari.Common tests against the staging code, without Azure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The two nested fixtures were named for an operating-system split (WhenUsingAWindowsDotNetAppService / WhenUsingALinuxAppService), but Calamari only branches on OS inside container deploy - which is now unit-tested. What these fixtures actually differ by is the resource type: a plain Web App versus a (Linux) Function App, both deployed through the same zip-deploy path. Rename them to WhenDeployingToAWebAzureApp and WhenDeployingToAFunctionAzureApp and add a short comment so the fixtures describe what they exercise. Also strip the file's leading byte-order mark. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AppServiceSettingsBehaviourFixture and AzureAppHealthCheckActionHandlerFixture were the only files in the folder still carrying a UTF-8 byte-order mark, which triggered an editor encoding warning. The repo convention is no-BOM, so remove it - the whole folder is now uniformly no-BOM. No content change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ExternalCloudIntegration category now needs to run as its own CI step: the main flavour test run should stay credential-free, and the live-Azure smoke suite should run separately. Exclude TestCategory=ExternalCloudIntegration from TestCalamariFlavourProject, and add a TestCalamariExternalCloudIntegrations target that runs only those tests across every flavour. Both combine with, rather than overwrite, any caller-supplied VSTest_TestCaseFilter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jtango18
reviewed
Jun 25, 2026
Jtango18
left a comment
Jtango18
left a comment
Contributor
There was a problem hiding this comment.
This looks like a great step forward to me.
Leaving comment only for now so others also have time to review
This can be ignored as its a reverse of the first commit
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What this does
Splits
Calamari.AzureAppService.Testsinto two clear groups: fast, credential-free unit tests of Calamari's own logic, and thinExternalCloudIntegrationsmoke tests that hit real Azure. Where a test only needed Azure incidentally, the single Azure call is pulled behind a seam (interface) so the surrounding logic can be unit-tested with a mock, and the cloud fixture shrinks to proving auth + the ARM round-trip still work. Redundant cloud tests are removed. Finally, the Nuke build is taught to run the two groups as separate CI steps.Each change is its own commit, so the PR is best reviewed commit-by-commit (each compiles and is self-contained).
Commits (in order)
596706bCategorise Azure tests that hit real cloud services — addTestCategory.ExternalCloudIntegrationand tag the real-cloud base classes so CI can run the credential-free suite separately.8772b60Extract IAzureWebAppDiscoverer seam and unit-test target discovery — pull the Resource Graph query behind an interface; move the 5 tag-matching tests to credential-free unit tests.71e5e71Consolidate App Service cloud test bases and group under ExternalCloudIntegration — collapse to a 3-level base hierarchy (shared auth → provisioned vs static resources) and move the cloud fixtures into the folder.dfd053dExtract IAzureAppServiceContainerConfigurer and unit-test container deploy — mock the container Azure calls; unit-test the OS branch + slot targeting and drop the redundant Windows cloud fixture.6476709Extract PackageProviderFactory and unit-test package routing — unit-test the extension → provider / upload-URL / sync-async routing instead of via cloud deploys.4a5a3a1Drop the WAR and JAR cloud deploy tests — both route throughJavaPackageProviderdiffering only by upload URL, now covered by the factory test.1cd0132Remove the no-op async-deployment-and-polling duplicate tests — their arrange was a no-op; the async path is already exercised by the baseline zip/nuget tests.0854784Remove the misnamed DeployToTwoTargetsInParallel cloud test — a generic file-share/package-staging concern, not Azure's; belongs inCalamari.Common.60bac5cReframe the deploy fixtures as Web App vs Function App — the split is resource type, not OS; rename the fixtures to match.caf6fecStrip BOM from two ExternalCloudIntegration fixtures — bring the last two BOM-carrying files into line with the repo's no-BOM convention (no content change, essentially No-Op commit)6b331c9Add a separate build target for external cloud integration tests — excludeExternalCloudIntegrationfrom the main flavour test run and add aTestCalamariExternalCloudIntegrationsNuke target that runs only those tests across every flavour.Testing
dotnet test --filter "TestCategory!=ExternalCloudIntegration"→ 30/30.ExternalCloudIntegrationfixtures were not run (no Azure creds in dev) — verified by build + review only. The splitOneTimeSetUpordering in the consolidated base classes (base authenticates, derived provisions/looks-up) wants one real CI run to confirm.