Centralize Visual Studio installation discovery and assembly resolution in test infrastructure #19085

Copilot · 2025-11-13T14:01:29Z

Description

Consolidates VS installation discovery and assembly resolution logic scattered across test files. Four files had duplicate fallback code; VsMocks.fs hardcoded VS170COMNTOOLS instead of using centralized discovery. Two AssemblyResolver.fs files contained identical assembly resolution logic.

Changes

Added to VSInstallDiscovery.fs:

getVSInstallDirOrFail() - fails with detailed error showing discovery methods attempted
VSAssemblyResolver module - centralizes assembly resolution logic for VS integration tests (named to avoid conflict with existing AssemblyResolver module in CompilerAssert.fs)

Updated consumer files:

Removed ~116 lines of duplicate code across files (net reduction of 49 lines)
Both AssemblyResolver.fs files now delegate to centralized FSharp.Test.VSAssemblyResolver module
VsMocks.fs uses centralized discovery via linked VSInstallDiscovery.fs file
Fixed unused parameter warnings (fun h args → fun _ args)

Project structure changes:

Added VSInstallDiscovery.fs as linked file to Salsa project (avoids NuGet reference warnings)
Suppressed warning 44 for deprecated AssemblyName.CodeBase property (still needed for assembly resolution)

Impact

VS version updates (e.g., VS 2026) require changes in one place only
Assembly resolution logic shared across all VS integration test projects
Error messages now show all discovery attempts (VSAPPIDDIR, VS*COMNTOOLS, vswhere.exe)
No circular dependencies - Salsa project uses linked file instead of package reference
Test infrastructure uses identical discovery and resolution logic across all files

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:

NO_RELEASE_NOTES - Internal test infrastructure refactoring only, no user-facing changes

Original prompt

Unify Visual Studio Installation Discovery Across Test Infrastructure

Overview

Currently, the codebase has Visual Studio installation discovery logic duplicated across multiple test files, with some files still hardcoding specific VS version environment variables (like VS170COMNTOOLS). We have a centralized VSInstallDiscovery.fs module that handles this properly, but not all files are using it correctly.

Goals

Remove redundant fallback logic from all consumer files
Add a new helper function to centralize error handling
Update all files to use the centralized discovery mechanism
Ensure consistent formatting and error messages across all files

Current Problems

Duplicated fallback code: Three files have identical 8-line fallback logic that's redundant
Hardcoded version: VsMocks.fs still hardcodes VS170COMNTOOLS instead of using centralized discovery
Inconsistent error messages: Different files fail with different messages
Poor diagnostics: Error messages don't explain what discovery methods were tried

Files to Modify

1. `tests/FSharp.Test.Utilities/VSInstallDiscovery.fs`

Add a new helper function at the end of the file (after the existing getVSInstallDirWithLogging function):

    /// Gets the VS installation directory or fails with a detailed error message.
    /// This is the recommended method for test scenarios that require VS to be installed.
    let getVSInstallDirOrFail () : string =
        match tryFindVSInstallation () with
        | Found (path, _) -> path
        | NotFound reason -> 
            failwith $"Visual Studio installation not found: {reason}. Ensure VS is installed or environment variables (VSAPPIDDIR, VS*COMNTOOLS) are set."

This should be added after line 128 (after the getVSInstallDirWithLogging function).

2. `vsintegration/tests/UnitTests/AssemblyResolver.fs`

Replace the entire file content with:

namespace Microsoft.VisualStudio.FSharp

open System
open System.IO
open System.Reflection

module AssemblyResolver =
    open System.Globalization
    open FSharp.Test.VSInstallDiscovery

    let vsInstallDir = getVSInstallDirOrFail ()

    let probingPaths =
        [|
            Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\Editor")
            Path.Combine(vsInstallDir, @"IDE\PublicAssemblies")
            Path.Combine(vsInstallDir, @"IDE\PrivateAssemblies")
            Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\ManagedLanguages\VBCSharp\LanguageServices")
            Path.Combine(vsInstallDir, @"IDE\Extensions\Microsoft\CodeSense\Framework")
            Path.Combine(vsInstallDir, @"IDE")
        |]

    let addResolver () =
        AppDomain.CurrentDomain.add_AssemblyResolve(fun _ args ->
            let found () =
                probingPaths
                |> Seq.tryPick (fun p ->
                    try
                        let name = AssemblyName(args.Name)
                        let codebase = Path.GetFullPath(Path.Combine(p, name.Name) + ".dll")
                        if File.Exists(codebase) then
                            name.CodeBase <- codebase
                            name.CultureInfo <- Unchecked.defaultof<CultureInfo>
                            name.Version <- Unchecked.defaultof<Version>
                            Some name
                        else
                            None
                    with _ ->
                        None)

            match found () with
            | None -> Unchecked.defaultof<Assembly>
            | Some name -> Assembly.Load(name))

Key changes from original:

Removed lines 11-24 (redundant fallback logic)
Changed line 11 from match tryGetVSInstallDir()... to let vsInstallDir = getVSInstallDirOrFail ()
Changed fun h args -> to fun _ args -> (unused parameter)
Removed extra spaces in (probingPaths ) → probingPaths
Consistent formatting

3. `vsintegration/tests/FSharp.Editor.Tests/Helpers/AssemblyResolver.fs`

Replace the entire file content with:

// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.

namespace FSharp.Editor.Tests.Helpers

open System
open System.IO
open System.Reflection

module AssemblyResolver =
    open System.Globalization
    open FSharp.Test.VSInstallDiscovery

    let vsInstallDir = getVSInstallDirOrFail ()

    let probingPaths =
        [|
            Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\Editor")
            Path.Combine(vsInstallDir, @"IDE\PublicAssemblies")
            Path.Combine(vsInstallDir, @"IDE\PrivateAssemblies")
            Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\ManagedLanguages\VBCSharp\LanguageServices")
            Path.Combine(vsInstallDir, @"IDE\Extensions\Microsoft\CodeSense\Framework")
            Path.Combine(vsInstallDir, @"IDE")
        |]

    let addResolver () =
        App...

</details>

*This pull request was created as a result of the following prompt from Copilot chat.*
> 
> # Unify Visual Studio Installation Discovery Across Test Infrastructure
> 
> ## Overview
> 
> Currently, the codebase has Visual Studio installation discovery logic duplicated across multiple test files, with some files still hardcoding specific VS version environment variables (like `VS170COMNTOOLS`). We have a centralized `VSInstallDiscovery.fs` module that handles this properly, but not all files are using it correctly.
> 
> ## Goals
> 
> 1. Remove redundant fallback logic from all consumer files
> 2. Add a new helper function to centralize error handling
> 3. Update all files to use the centralized discovery mechanism
> 4. Ensure consistent formatting and error messages across all files
> 
> ## Current Problems
> 
> - **Duplicated fallback code**: Three files have identical 8-line fallback logic that's redundant
> - **Hardcoded version**: `VsMocks.fs` still hardcodes `VS170COMNTOOLS` instead of using centralized discovery
> - **Inconsistent error messages**: Different files fail with different messages
> - **Poor diagnostics**: Error messages don't explain what discovery methods were tried
> 
> ## Files to Modify
> 
> ### 1. `tests/FSharp.Test.Utilities/VSInstallDiscovery.fs`
> 
> Add a new helper function at the end of the file (after the existing `getVSInstallDirWithLogging` function):
> 
> ```fsharp
>     /// Gets the VS installation directory or fails with a detailed error message.
>     /// This is the recommended method for test scenarios that require VS to be installed.
>     let getVSInstallDirOrFail () : string =
>         match tryFindVSInstallation () with
>         | Found (path, _) -> path
>         | NotFound reason -> 
>             failwith $"Visual Studio installation not found: {reason}. Ensure VS is installed or environment variables (VSAPPIDDIR, VS*COMNTOOLS) are set."
> ```
> 
> This should be added after line 128 (after the `getVSInstallDirWithLogging` function).
> 
> ### 2. `vsintegration/tests/UnitTests/AssemblyResolver.fs`
> 
> Replace the entire file content with:
> 
> ```fsharp
> namespace Microsoft.VisualStudio.FSharp
> 
> open System
> open System.IO
> open System.Reflection
> 
> module AssemblyResolver =
>     open System.Globalization
>     open FSharp.Test.VSInstallDiscovery
> 
>     let vsInstallDir = getVSInstallDirOrFail ()
> 
>     let probingPaths =
>         [|
>             Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\Editor")
>             Path.Combine(vsInstallDir, @"IDE\PublicAssemblies")
>             Path.Combine(vsInstallDir, @"IDE\PrivateAssemblies")
>             Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\ManagedLanguages\VBCSharp\LanguageServices")
>             Path.Combine(vsInstallDir, @"IDE\Extensions\Microsoft\CodeSense\Framework")
>             Path.Combine(vsInstallDir, @"IDE")
>         |]
> 
>     let addResolver () =
>         AppDomain.CurrentDomain.add_AssemblyResolve(fun _ args ->
>             let found () =
>                 probingPaths
>                 |> Seq.tryPick (fun p ->
>                     try
>                         let name = AssemblyName(args.Name)
>                         let codebase = Path.GetFullPath(Path.Combine(p, name.Name) + ".dll")
>                         if File.Exists(codebase) then
>                             name.CodeBase <- codebase
>                             name.CultureInfo <- Unchecked.defaultof<CultureInfo>
>                             name.Version <- Unchecked.defaultof<Version>
>                             Some name
>                         else
>                             None
>                     with _ ->
>                         None)
> 
>             match found () with
>             | None -> Unchecked.defaultof<Assembly>
>             | Some name -> Assembly.Load(name))
> ```
> 
> **Key changes from original:**
> - Removed lines 11-24 (redundant fallback logic)
> - Changed line 11 from `match tryGetVSInstallDir()...` to `let vsInstallDir = getVSInstallDirOrFail ()`
> - Changed `fun h args ->` to `fun _ args ->` (unused parameter)
> - Removed extra spaces in `(probingPaths )` → `probingPaths`
> - Consistent formatting
> 
> ### 3. `vsintegration/tests/FSharp.Editor.Tests/Helpers/AssemblyResolver.fs`
> 
> Replace the entire file content with:
> 
> ```fsharp
> // Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
> 
> namespace FSharp.Editor.Tests.Helpers
> 
> open System
> open System.IO
> open System.Reflection
> 
> module AssemblyResolver =
>     open System.Globalization
>     open FSharp.Test.VSInstallDiscovery
> 
>     let vsInstallDir = getVSInstallDirOrFail ()
> 
>     let probingPaths =
>         [|
>             Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\Editor")
>             Path.Combine(vsInstallDir, @"IDE\PublicAssemblies")
>             Path.Combine(vsInstallDir, @"IDE\PrivateAssemblies")
>             Path.Combine(vsInstallDir, @"IDE\CommonExtensions\Microsoft\ManagedLanguages\VBCSharp\LanguageServices")
>             Path.Combine(vsInstallDir, @"IDE\Extensions\Microsoft\CodeSense\Framework")
>             Path.Combine(vsInstallDir, @"IDE")
>         |]
> 
>     let addResolver () =
>         AppDomain.CurrentDomain.add_AssemblyResolve(fun _ args ->
>             let found () =
>                 probingPaths
>                 |> Seq.tryPick (fun p ->
>                     try
>                         let name = AssemblyName(args.Name)
>                         let codebase = Path.GetFullPath(Path.Combine(p, name.Name) + ".dll")
>                         if File.Exists(codebase) then
>                             name.CodeBase <- codebase
>                             name.CultureInfo <- Unchecked.defaultof<CultureInfo>
>                             name.Version <- Unchecked.defaultof<Version>
>                             Some name
>                         else
>                             None
>                     with _ ->
>                         None)
> 
>             match found () with
>             | None -> Unchecked.defaultof<Assembly>
>             | Some name -> Assembly.Load(name))
> ```
> 
> **Key changes from original:**
> - Removed lines 13-28 (redundant fallback logic)
> - Changed line 13 from `match tryGetVSInstallDir()...` to `let vsInstallDir = getVSInstallDirOrFail ()`
> - Changed `fun h args ->` to `fun _ args ->` (unused parameter)
> - Consistent formatting
> 
> ### 4. `vsintegration/tests/Salsa/VsMocks.fs`
> 
> Find the `module internal VsActual` section (around line 1633) and update the `vsInstallDir` definition. 
> 
> **Current code (lines ~1633-1653):**
> ```fsharp
> module internal VsActual = 
>     open System.Reflection
>     open System.IO
>     open System.ComponentModel.Composition.Hosting
>     open System.ComponentModel.Composition.Primitives
>     open Microsoft.VisualStudio.Text
>     open Microsoft.VisualStudio.Threading
> 
>     type TestExportJoinableTaskContext () =
>         static let jtc = new JoinableTaskContext()
> 
>         [<System.ComponentModel.Composition.Export(typeof<JoinableTaskContext>)>]
>         member public _.JoinableTaskContext : JoinableTaskContext = jtc
> 
>     let vsInstallDir =
>         // use the environment variable to find the VS installdir
>         let vsvar =
>             let var = Environment.GetEnvironmentVariable("VS170COMNTOOLS")
>             if String.IsNullOrEmpty var then
>                 Environment.GetEnvironmentVariable("VSAPPIDDIR")
>             else
>                 var
>         if String.IsNullOrEmpty vsvar then failwith "VS170COMNTOOLS and VSAPPIDDIR environment variables not found."
>         Path.Combine(vsvar, "..")
> ```
> 
> **Replace with:**
> ```fsharp
> module internal VsActual = 
>     open System.Reflection
>     open System.IO
>     open System.ComponentModel.Composition.Hosting
>     open System.ComponentModel.Composition.Primitives
>     open Microsoft.VisualStudio.Text
>     open Microsoft.VisualStudio.Threading
>     open FSharp.Test.VSInstallDiscovery
> 
>     type TestExportJoinableTaskContext () =
>         static let jtc = new JoinableTaskContext()
> 
>         [<System.ComponentModel.Composition.Export(typeof<JoinableTaskContext>)>]
>         member public _.JoinableTaskContext : JoinableTaskContext = jtc
> 
>     let vsInstallDir = getVSInstallDirOrFail ()
> ```
> 
> **Key changes:**
> - Add `open FSharp.Test.VSInstallDiscovery` to the module opens
> - Replace the entire `vsInstallDir` binding (lines with hardcoded `VS170COMNTOOLS`) with single line: `let vsInstallDir = getVSInstallDirOrFail ()`
> 
> ## Benefits of These Changes
> 
> ✅ **Single source of truth**: All VS version environment variables (`VS180COMNTOOLS`, `VS170COMNTOOLS`, etc.) are only referenced in `VSInstallDiscovery.fs`
> 
> ✅ **Automatic forward compatibility**: When VS 2026 ships, only the central list needs updating
> 
> ✅ **Better error messages**: Users now see which discovery methods were tried (VSAPPIDDIR, VS*COMNTOOLS, vswhere.exe)
> 
> ✅ **Reduced code duplication**: Removed ~40 lines of redundant fallback logic across files
> 
> ✅ **Consistent behavior**: All test infrastructure uses the same discovery logic with same priority order
> 
> ## Testing
> 
> After making these changes:
> 1. Build the solution to ensure no compilation errors
> 2. Run the existing tests to ensure VS installation discovery still works
> 3. The discovery priority order remains: VSAPPIDDIR → VS*COMNTOOLS (trying highest version first) → vswhere.exe
> 
> ## Notes for Implementation
> 
> - The `VSInstallDiscovery.fs` already has the smart discovery logic - we're just adding one helper function
> - The centralized discovery already tries VSAPPIDDIR first (best when running inside VS), then all VS*COMNTOOLS versions (not just VS170), then vswhere.exe
> - All error handling is now centralized in the new `getVSInstallDirOrFail()` function
> - No behavioral changes - just removing redundant code and improving error messages
> 

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/dotnet/fsharp/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: T-Gro <[email protected]>

vsintegration/tests/Salsa/VsMocks.fs

- Move AssemblyResolver logic to VSInstallDiscovery.fs as a new module - Update both AssemblyResolver.fs files to delegate to centralized implementation - Add VSInstallDiscovery.fs as linked file to Salsa project (fixes missing reference) - Reduces code duplication further (~70 lines total removed) Co-authored-by: T-Gro <[email protected]>

The AssemblyName.CodeBase property is deprecated but still needed for assembly resolution in VS integration tests. Suppress warning 44 in FSharp.Test.Utilities project. Co-authored-by: T-Gro <[email protected]>

github-actions · 2025-11-14T15:45:49Z

✅ No release notes required

T-Gro

@copilot :

Pls rename it to avoid conflicts.

D:\a_work\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs(32,8): error FS0248: Two modules named 'FSharp.Test.AssemblyResolver' occur in two parts of this assembly [D:\a_work\1\s\tests\FSharp.Test.Utilities\FSharp.Test.Utilities.fsproj::TargetFramework=net472]

D:\a_work\1\s\tests\FSharp.Test.Utilities\VSInstallDiscovery.fs(140,8): error FS0248: Two modules named 'FSharp.Test.AssemblyResolver' occur in two parts of this assembly [D:\a_work\1\s\tests\FSharp.Test.Utilities\FSharp.Test.Utilities.fsproj::TargetFramework=net472]
0 Warning(s)
2 Error(s)

Fixes FS0248 error: Two modules named 'FSharp.Test.AssemblyResolver' were occurring because CompilerAssert.fs already has an AssemblyResolver module. Renamed the new module to VSAssemblyResolver to avoid conflict. Co-authored-by: T-Gro <[email protected]>

Copilot · 2025-11-27T13:35:23Z

@copilot :

Pls rename it to avoid conflicts....

Fixed in 86177e3. Renamed the module from AssemblyResolver to VSAssemblyResolver to avoid the conflict with the existing AssemblyResolver module in CompilerAssert.fs.

T-Gro · 2025-11-27T14:54:58Z

/azp run

azure-pipelines · 2025-11-27T14:55:10Z

Azure Pipelines successfully started running 2 pipeline(s).

Initial plan

0d34666

Copilot AI assigned Copilot and T-Gro Nov 13, 2025

github-project-automation bot added this to F# Compiler and Tooling Nov 13, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling Nov 13, 2025

Copilot started work on behalf of T-Gro November 13, 2025 14:01 View session

Unify VS install discovery - add helper and update all files

3dbb449

Co-authored-by: T-Gro <[email protected]>

Copilot AI changed the title ~~[WIP] Remove duplicated Visual Studio installation discovery logic~~ Centralize Visual Studio installation discovery in test infrastructure Nov 13, 2025

Copilot AI requested a review from T-Gro November 13, 2025 14:46

Copilot finished work on behalf of T-Gro November 13, 2025 14:46

T-Gro reviewed Nov 13, 2025

View reviewed changes

vsintegration/tests/Salsa/VsMocks.fs Show resolved Hide resolved

Copilot started work on behalf of T-Gro November 13, 2025 17:50 View session

Copilot AI and others added 2 commits November 13, 2025 18:03

Suppress warning 44 for deprecated CodeBase property

19196b7

The AssemblyName.CodeBase property is deprecated but still needed for assembly resolution in VS integration tests. Suppress warning 44 in FSharp.Test.Utilities project. Co-authored-by: T-Gro <[email protected]>

Copilot AI changed the title ~~Centralize Visual Studio installation discovery in test infrastructure~~ Centralize Visual Studio installation discovery and assembly resolution in test infrastructure Nov 13, 2025

Copilot AI requested a review from T-Gro November 13, 2025 18:21

Copilot finished work on behalf of T-Gro November 13, 2025 18:21

Merge branch 'main' into copilot/unify-visual-studio-discovery

8b9b796

T-Gro approved these changes Nov 14, 2025

View reviewed changes

T-Gro requested a review from abonie November 14, 2025 15:57

T-Gro marked this pull request as ready for review November 14, 2025 15:57

T-Gro requested a review from a team as a code owner November 14, 2025 15:57

Merge branch 'main' into copilot/unify-visual-studio-discovery

5811c14

T-Gro requested changes Nov 27, 2025

View reviewed changes

github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Nov 27, 2025

Copilot started work on behalf of T-Gro November 27, 2025 13:22 View session

Copilot AI requested a review from T-Gro November 27, 2025 13:37

Copilot finished work on behalf of T-Gro November 27, 2025 13:37

Merge branch 'main' into copilot/unify-visual-studio-discovery

eb4d5f2

T-Gro approved these changes Nov 27, 2025

View reviewed changes

T-Gro requested review from abonie and removed request for abonie November 27, 2025 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Centralize Visual Studio installation discovery and assembly resolution in test infrastructure #19085

Centralize Visual Studio installation discovery and assembly resolution in test infrastructure #19085

Copilot AI commented Nov 13, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

T-Gro left a comment

Uh oh!

Copilot AI commented Nov 27, 2025

Uh oh!

T-Gro commented Nov 27, 2025

Uh oh!

azure-pipelines bot commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Centralize Visual Studio installation discovery and assembly resolution in test infrastructure #19085

Are you sure you want to change the base?

Centralize Visual Studio installation discovery and assembly resolution in test infrastructure #19085

Conversation

Copilot AI commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Impact

Checklist

Unify Visual Studio Installation Discovery Across Test Infrastructure

Overview

Goals

Current Problems

Files to Modify

1. tests/FSharp.Test.Utilities/VSInstallDiscovery.fs

2. vsintegration/tests/UnitTests/AssemblyResolver.fs

3. vsintegration/tests/FSharp.Editor.Tests/Helpers/AssemblyResolver.fs

Uh oh!

Uh oh!

github-actions bot commented Nov 14, 2025

✅ No release notes required

Uh oh!

T-Gro left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 27, 2025

Uh oh!

T-Gro commented Nov 27, 2025

Uh oh!

azure-pipelines bot commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Nov 13, 2025 •

edited

Loading

1. `tests/FSharp.Test.Utilities/VSInstallDiscovery.fs`

2. `vsintegration/tests/UnitTests/AssemblyResolver.fs`

3. `vsintegration/tests/FSharp.Editor.Tests/Helpers/AssemblyResolver.fs`