Support compliant 1ES build agents and refactor test skipping behavior#5910
Open
Avery-Dunn wants to merge 21 commits intomainfrom
Open
Support compliant 1ES build agents and refactor test skipping behavior#5910Avery-Dunn wants to merge 21 commits intomainfrom
Avery-Dunn wants to merge 21 commits intomainfrom
Conversation
…ation-library-for-dotnet into avdunn/agent-pool-fix
Contributor
There was a problem hiding this comment.
Pull request overview
Updates MSAL.NET’s Azure DevOps build/test templates and test infrastructure to work on standard 1ES agents (and OneBranch’s different source layout) while fixing previously fragile/ineffective test-skipping behavior by consolidating skip logic into RunOnAttribute.
Changes:
- Pipeline templates: introduce/propagate
MsalSourceDirandPipelineTypeto fix path resolution across OneBranch vs non-OneBranch agents. - Test infra: extend
[RunOn]with a new[Flags] SkipConditionsenum to support composable skip logic (OneBranch build, OS, federated-disabled, etc.) and remove obsolete attributes. - Tests: refactor Selenium- and broker-related tests to use the new skip conditions (e.g., skip on OneBranch builds, Linux, federated-disabled), removing
#if/stacked attributes where applicable.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/TargetFramework.cs | Adds SkipConditions and new RunOnAttribute overload to centralize multi-condition skipping. |
| tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/IgnoreOnOneBranch.cs | Removes obsolete OneBranch-only ignore attribute. |
| tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/IgnoreFederatedTests.cs | Removes obsolete federated-only ignore attribute. |
| tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/SSHCertificateTests.cs | Converts Selenium test to [RunOn(SkipConditions.OneBranchBuild)]. |
| tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/DeviceCodeFlowIntegrationTest.cs | Converts device-code Selenium tests to skip on OneBranch builds. |
| tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/ConfidentialClientAuthorizationTests.cs | Converts auth-code Selenium tests to skip on OneBranch builds. |
| tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/InteractiveFlowTests.NetFwk.cs | Adds OneBranch + federated-disabled skip conditions via [RunOn(...)]. |
| tests/Microsoft.Identity.Test.Integration.netcore/SeleniumTests/SeleniumInfrastructureTests.NetFwk.cs | Skips Selenium infra failure-mode tests on OneBranch builds. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/RuntimeBrokerTests.cs | Replaces stacked skip attributes with combined SkipConditions on [RunOn]. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/PoPTests.NetFwk.cs | Replaces Linux-skip attributes with [RunOn(..., SkipConditions.Linux)] or [RunOn(SkipConditions.Linux)]. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsMtlsPopTests.cs | Consolidates Linux skip into [RunOn(SkipConditions.Linux)] to avoid attribute stacking issues. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/UsernamePasswordIntegrationTests.NetFwk.cs | Replaces #if IGNORE_FEDERATED ignore blocks with SkipConditions.FederatedDisabled. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs | Adds OneBranch-specific inconclusive path for IMDS-based region auto-detection. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/CiamIntegrationTests.cs | Marks select CIAM tests to skip on OneBranch builds via [RunOn]. |
| build/pipeline-pullrequest.yaml | Defines MsalSourceDir: '' for non-OneBranch pipelines to avoid literal-path issues. |
| build/template-OneBranch-Tests-libsandsamples.yaml | Prefixes solution path with MsalSourceDir and passes through MsalSourceDir/PipelineType. |
| build/template-OneBranch-CI-libsandsamples.yaml | Adds PipelineType MSBuild arg so tests can define ONEBRANCH_BUILD. |
| build/template-run-unit-tests.yaml | Renames display names; updates runsettings path to include $(MsalSourceDir) for .NET 8 run. |
| build/template-run-integration-tests.yaml | Renames display names; updates runsettings path to include $(MsalSourceDir) for .NET 8 run. |
tests/Microsoft.Identity.Test.Integration.netcore/Infrastructure/TargetFramework.cs
Show resolved
Hide resolved
RyAuld
approved these changes
Apr 6, 2026
neha-bhargava
approved these changes
Apr 7, 2026
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
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.
Resolves #5881
This PR is the companion to the OneBranch pipeline repo changes that switch from custom to standard 1ES build agents, and both must be merged in at the same time to avoid breaking our pipelines: https://identitydivision.visualstudio.com/IDDP/_git/MSAL.NET-OneBranch/pullrequest/22988
The main goal of this PR is to updates pipeline templates to handle path differences between OneBranch and non-OneBranch agents, however it also fixes latent bugs in how tests are conditionally skipped which were discovered while making those changes.
In order to make our tests work on standard 1ES build agents, the following changes were made:
template-OneBranch-Tests-libsandsamples.yaml: Adjusted directory paths to include the$(MsalSourceDir)prefix which is needed on OneBranch builds to properly find the source code, and passes MsalSourceDir and PipelineType parameters through to sub-templates-
pipeline-pullrequest.yaml: AddedMsalSourceDir: ''so the variable resolves to empty in the non-OneBranch pipeline. Without explicitly setting it to empty, certain paths attempt to use a literal$MsalSourceDir/as the root directory nametemplate-OneBranch-CI-libsandsamples.yaml: Added PipelineType parameter, passes soDirectory.Build.propscan define theONEBRANCH_BUILDcondition properly in both OneBranch and non-OneBranch scenarios.$(MsalSourceDir)prefix torunSettingsFilepaths in OneBranch-only tasks, so they resolve correctly under OneBranch's subdirectory layoutONEBRANCH_BUILDvariable to all tests that rely on Selenium/browser-based testing, as we cannot get the necessary dependencies reliably in a OneBranch build (the "official"/release template seems to allow Chrome installation, but not ChromeDriver)While implementing the above YAML changes here and in our Azure DevOps repo, I noticed a lot of weird behavior in how tests were running and being skipped in our pipelines via custom attributes like
[IgnoreOnOneBranch]. In short, this condition is dead code and there are a couple latent bugs around test skipping behavior in our current pipeline infrastructure:PipelineTypevariable being set toOneBranch, however our only OneBranch pipeline that runs the tests hardcodesPipelineTypeto "Legacy", meaning the OneBranch path never happens[IgnoreOnOneBranch]and[DoNotRunOnLinux]are stacked on the same test, only first attribute defined on a test matters and the others are ignoredMore context and clearer proof of these latent bugs can be seen in this draft PR #5900 (which will not be needed anymore, as this PR implements the same fix and to all relevant tests).
So in addition to the more straightforward parameter/directory path changes in the YAML templates, this PR also makes a few fixes and code quality improvements to our test infrastructure:
TargetFramework: Extended the existingRunOnAttribute/[RunOn(...)]behavior to support a new enum ofSkipConditions:OneBranchBuild: Skip whenONEBRANCH_BUILDis defined at compile time (what[IgnoreOnOneBranch]was intended to do)Linux/Windows/macOS: Skip on the specified OS at runtime (what[DoNotRunOnLinux]currently does)FederatedDisabled: Skip whenIGNORE_FEDERATEDis defined at compile-time (what#if IGNORE_FEDERATEDcurrently does)NotAzureDevOps: Skip whenTF_BUILDenvironment variable is absent at runtime (might be vestigial?)IgnoreOnOneBranch,IgnoreFederatedTests: removed entirely, their behavior was fully handled by the changes toTargetFramework[TestMethod]removal: Custom attributes like[RunOn(...)]extend the same sort of classes as[TestMethod], so it is not needed if a custom attribute is used#if ONEBRANCH_BUILD/#if IGNORE_FEDERATED wrapping: Replaced compile-time #if guards around entire test methods with attribute-level skip conditions via[RunOn(...)]In short
[RunOn(...)]now accepts an optionalSkipConditionsparameter alongside the existingTargetFrameworks, and evaluates all conditions in a single pass. This resolves the issue with stacking custom attributes/skip conditions, and for the vast majority tests which have no conditions or only use ``[RunOn(TargetFrameworks.XYZ)]` alone no change was needed.Finally, to help demonstrate that these changes work without affecting the intended test behavior (i.e., unexpectedly skipping tests that can run), here are some pipeline runs for comparison: