Skip to content

Support compliant 1ES build agents and refactor test skipping behavior#5910

Open
Avery-Dunn wants to merge 21 commits intomainfrom
avdunn/agent-pool-fix
Open

Support compliant 1ES build agents and refactor test skipping behavior#5910
Avery-Dunn wants to merge 21 commits intomainfrom
avdunn/agent-pool-fix

Conversation

@Avery-Dunn
Copy link
Copy Markdown
Contributor

@Avery-Dunn Avery-Dunn commented Apr 6, 2026

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: Added MsalSourceDir: '' 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 name
  • template-OneBranch-CI-libsandsamples.yaml: Added PipelineType parameter, passes so Directory.Build.props can define the ONEBRANCH_BUILD condition properly in both OneBranch and non-OneBranch scenarios.
  • template-run-unit-tests.yaml / template-run-integration-tests.yaml:
    • Added $(MsalSourceDir) prefix to runSettingsFile paths in OneBranch-only tasks, so they resolve correctly under OneBranch's subdirectory layout
    • Renamed all 6 test task display names for clarity (unrelated to the core changes, just a code quality/readability issue I noticed)
  • Various test classes: added skip conditions based on the ONEBRANCH_BUILD variable 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:

  • It relies on a PipelineType variable being set to OneBranch, however our only OneBranch pipeline that runs the tests hardcodes PipelineType to "Legacy", meaning the OneBranch path never happens
  • When custom attributes like [IgnoreOnOneBranch] and [DoNotRunOnLinux] are stacked on the same test, only first attribute defined on a test matters and the others are ignored

More 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 existing RunOnAttribute/[RunOn(...)] behavior to support a new enum of SkipConditions:
    • OneBranchBuild: Skip when ONEBRANCH_BUILD is 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 when IGNORE_FEDERATED is defined at compile-time (what #if IGNORE_FEDERATED currently does)
    • NotAzureDevOps: Skip when TF_BUILD environment variable is absent at runtime (might be vestigial?)
  • IgnoreOnOneBranch, IgnoreFederatedTests: removed entirely, their behavior was fully handled by the changes to TargetFramework
  • Redundant [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
  • Removed #if ONEBRANCH_BUILD/#if IGNORE_FEDERATED wrapping: Replaced compile-time #if guards around entire test methods with attribute-level skip conditions via [RunOn(...)]
  • Various test classes: refactored to use the new behavior described above

In short [RunOn(...)] now accepts an optional SkipConditions parameter alongside the existing TargetFrameworks, 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:

  • Non-OneBranch PR pipeline
    • Last successful scheduled run on main, build ID 1616122: 3948 total tests found (3,855 Passed, 0 Failed, 93 Others)
    • Successful run with these changes, build ID 1616806: 3948 total tests found (3,852 Passed, 0 Failed, 96 Others)
  • OneBranch release pipeline
    • Last successful scheduled run on main, build ID 1616179: 3929 total tests found (3,837 Passed, 0 Failed, 92 Others)
    • Successful run with these changes, build ID 1616805: 3929 total tests found (3,799 Passed, 0 Failed, 130 Others)

Copilot AI review requested due to automatic review settings April 6, 2026 23:19
@Avery-Dunn Avery-Dunn marked this pull request as ready for review April 6, 2026 23:19
@Avery-Dunn Avery-Dunn requested a review from a team as a code owner April 6, 2026 23:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 MsalSourceDir and PipelineType to fix path resolution across OneBranch vs non-OneBranch agents.
  • Test infra: extend [RunOn] with a new [Flags] SkipConditions enum 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.

Copy link
Copy Markdown
Contributor

@gladjohn gladjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Engineering task] Please switch to using 1ES Hosted Pools by March 1st, 2026

5 participants