Add unit tests for toolchain selection

docs/contributor/writing-tests-for-vscode-swift.md

@@ -12,7 +12,6 @@ A brief description of each framework can be found below:
 
 - [Organizing Tests](#organizing-tests)
 - [Writing Unit Tests](#writing-unit-tests)
-- [Mocking the File System](#mocking-the-file-system)
 - [Mocking Utilities](#mocking-utilities)
   - [Mocking interfaces, classes, and functions](#mocking-interfaces-classes-and-functions)
   - [Mocking VS Code events](#mocking-vs-code-events)
@@ -21,6 +20,7 @@ A brief description of each framework can be found below:
     - [Mocking global events](#mocking-global-events)
     - [Setting global constants](#setting-global-constants)
     - [Mocking an entire module](#mocking-an-entire-module)
+    - [Mocking the File System](#mocking-the-file-system)
 - [Conclusion](#conclusion)
 
 ## Organizing Tests
@@ -114,44 +114,6 @@ suite("ReloadExtension Unit Test Suite", () => {
 
 You may have also noticed that we needed to cast the `"Reload Extensions"` string to `any` when resolving `showWarningMessage()`. Unforunately, this may be necessary for methods that have incompatible overloaded signatures due to a TypeScript issue that remains unfixed.
 
-## Mocking the File System
-
-Mocking file system access can be a challenging endeavor that is prone to fail when implementation details of the unit under test change. This is because there are many different ways of accessing and manipulating files, making it almost impossible to catch all possible failure paths. For example, you could check for file existence using `fs.stat()` or simply call `fs.readFile()` and catch errors with a single function call. Using the real file system is slow and requires extra setup code in test cases to configure.
-
-The [`mock-fs`](https://github.com/tschaub/mock-fs) module is a well-maintained library that can be used to mitigate these issues by temporarily replacing Node's built-in `fs` module with an in-memory file system. This can be useful for testing logic that uses the `fs` module without actually reaching out to the file system. Just a single function call can be used to configure what the fake file system will contain:
-
-```typescript
-import * as chai from "chai";
-import * as mockFS from "mock-fs";
-import * as fs from "fs/promises";
-
-suite("mock-fs example", () => {
-    // This teardown step is also important to make sure your tests clean up the
-    // mocked file system when they complete!
-    teardown(() => {
-        mockFS.restore();
-    });
-
-    test("mock out a file on disk", async () => {
-        // A single function call can be used to configure the file system
-        mockFS({
-            "/path/to/some/file": "Some really cool file contents",
-        });
-        await expect(fs.readFile("/path/to/some/file", "utf-8"))
-            .to.eventually.equal("Some really cool file contents");
-    });
-});
-```
-
-In order to test failure paths, you can either create an empty file system or use `mockFS.file()` to set the mode to make a file that is not accessible to the current user:
-
-```typescript
-test("file is not readable by the current user", async () => {
-    mockFS({ "/path/to/file": mockFS.file({ mode: 0o000 }) });
-    await expect(fs.readFile("/path/to/file", "utf-8")).to.eventually.be.rejected;
-});
-```
-
 ## Mocking Utilities
 
 This section outlines the various utilities that can be used to improve the readability of your tests. The [MockUtils](../../test/MockUtils.ts) module can be used to perform more advanced mocking than what Sinon provides out of the box. This module has its [own set of tests](../../test/unit-tests/MockUtils.test.ts) that you can use to get a feel for how it works.
@@ -362,6 +324,42 @@ suite("Mocked configuration example", async function () {
 });
 ```
 
+#### Mocking the File System
+
+Mocking file system access can be a challenging endeavor that is prone to fail when implementation details of the unit under test change. This is because there are many different ways of accessing and manipulating files, making it almost impossible to catch all possible failure paths. For example, you could check for file existence using `fs.stat()` or simply call `fs.readFile()` and catch errors with a single function call. Using the real file system is slow and requires extra setup code in test cases to configure.
+
+The [`mock-fs`](https://github.com/tschaub/mock-fs) module is a well-maintained library that can be used to mitigate these issues by temporarily replacing Node's built-in `fs` module with an in-memory file system. This can be useful for testing logic that uses the `fs` module without actually reaching out to the file system. Just a single function call can be used to configure what the fake file system will contain.
+
+Our mocking utility provides a function called `mockFileSystem()` that will handle `setup()` and `teardown()` of the `mock-fs` module automatically:
+
+```typescript
+import * as chai from "chai";
+import { mockFileSystem } from "../MockUtils";
+import * as fs from "fs/promises";
+
+suite("Mocked file system example", () => {
+    const mockFS = mockFileSystem();
+
+    test("mock out a file on disk", async () => {
+        // A single function call can be used to configure the file system
+        mockFS({
+            "/path/to/some/file": "Some really cool file contents",
+        });
+        await expect(fs.readFile("/path/to/some/file", "utf-8"))
+            .to.eventually.equal("Some really cool file contents");
+    });
+});
+```
+
+In order to test failure paths, you can either create an empty file system or use `mockFS.file()` to set the mode to make a file that is not accessible to the current user:
+
+```typescript
+test("file is not readable by the current user", async () => {
+    mockFS({ "/path/to/file": mockFS.file({ mode: 0o000 }) });
+    await expect(fs.readFile("/path/to/file", "utf-8")).to.eventually.be.rejected;
+});
+```
+
 ## Conclusion
 
 Writing clear and concise test cases is critical to ensuring the robustness of your contributions. By following the steps outlined in this document, you can create tests that are easy to understand, maintain, and extend.

src/toolchain/toolchain.ts

@@ -255,7 +255,7 @@ export class SwiftToolchain {
                 .filter((toolchain): toolchain is string => typeof toolchain === "string")
                 .map(toolchain => path.join(swiftlyHomeDir, "toolchains", toolchain));
         } catch (error) {
-            throw new Error("Failed to retrieve Swiftly installations from disk.");
+            return [];
         }
     }
 
@@ -271,7 +271,7 @@ export class SwiftToolchain {
         return Promise.all([
             this.findToolchainsIn("/Library/Developer/Toolchains/"),
             this.findToolchainsIn(path.join(os.homedir(), "Library/Developer/Toolchains/")),
-        ]).then(results => results.flatMap(a => a));
+        ]).then(results => results.flat());
     }
 
     /**

src/ui/ToolchainSelection.ts

@@ -17,6 +17,7 @@ import * as path from "path";
 import { showReloadExtensionNotification } from "./ReloadExtension";
 import { SwiftToolchain } from "../toolchain/toolchain";
 import configuration from "../configuration";
+import { Version } from "../utilities/version";
 
 /**
  * Open the installation page on Swift.org
@@ -133,6 +134,18 @@ class SeparatorItem implements vscode.QuickPickItem {
 /** The possible types of {@link vscode.QuickPickItem} in the toolchain selection dialog */
 type SelectToolchainItem = SwiftToolchainItem | ActionItem | SeparatorItem;
 
+function compareToolchainPaths(first: string, second: string): number {
+    first = path.basename(first, ".xctoolchain");
+    const firstVersion = Version.fromString(first) ?? new Version(9999, 9999, 9999);
+    second = path.basename(second, ".xctoolchain");
+    const secondVersion = Version.fromString(second) ?? new Version(9999, 9999, 9999);
+    const versionComparison = firstVersion.compare(secondVersion);
+    if (versionComparison === 0) {
+        return first.localeCompare(second);
+    }
+    return versionComparison;
+}
+
 /**
  * Retrieves all {@link SelectToolchainItem} that are available on the system.
  *
@@ -144,7 +157,7 @@ async function getQuickPickItems(
 ): Promise<SelectToolchainItem[]> {
     // Find any Xcode installations on the system
     const xcodes = (await SwiftToolchain.getXcodeInstalls())
-        .reverse()
+        .sort((a, b) => path.basename(a, ".app").localeCompare(path.basename(b, ".app")))
         .map<SwiftToolchainItem>(xcodePath => {
             const toolchainPath = path.join(
                 xcodePath,
@@ -166,6 +179,7 @@ async function getQuickPickItems(
         });
     // Find any public Swift toolchains on the system
     const toolchains = (await SwiftToolchain.getToolchainInstalls())
+        .sort(compareToolchainPaths)
         .reverse()
         .map<SwiftToolchainItem>(toolchainPath => {
             const result: SwiftToolchainItem = {
@@ -188,6 +202,7 @@ async function getQuickPickItems(
         });
     // Find any Swift toolchains installed via Swiftly
     const swiftlyToolchains = (await SwiftToolchain.getSwiftlyToolchainInstalls())
+        .sort(compareToolchainPaths)
         .reverse()
         .map<SwiftToolchainItem>(toolchainPath => ({
             type: "toolchain",
@@ -255,7 +270,7 @@ async function getQuickPickItems(
  *
  * @param activeToolchain the {@link WorkspaceContext}
  */
-export async function showToolchainSelectionQuickPick(activeToolchain: SwiftToolchain | undefined) {
+export async function showToolchainSelectionQuickPick(activeToolchain?: SwiftToolchain) {
     let xcodePaths: string[] = [];
     const selected = await vscode.window.showQuickPick<SelectToolchainItem>(
         getQuickPickItems(activeToolchain).then(result => {

src/utilities/version.ts

@@ -85,4 +85,14 @@ export class Version implements VersionInterface {
     isGreaterThanOrEqual(rhs: VersionInterface): boolean {
         return !this.isLessThan(rhs);
     }
+
+    compare(rhs: VersionInterface): number {
+        if (this.isGreaterThan(rhs)) {
+            return 1;
+        } else if (this.isLessThan(rhs)) {
+            return -1;
+        } else {
+            return 0;
+        }
+    }
 }

test/MockUtils.ts

@@ -13,6 +13,8 @@
 //===----------------------------------------------------------------------===//
 import * as vscode from "vscode";
 import { stub, SinonStub } from "sinon";
+import * as mockFS from "mock-fs";
+import FileSystem = require("mock-fs/lib/filesystem");
 
 /**
  * Waits for all promises returned by a MockedFunction to resolve. Useful when
@@ -127,6 +129,23 @@ function replaceWithMocks<T>(obj: Partial<T>): MockedObject<T> {
     return result;
 }
 
+function checkAndAcquireValueFromTarget(
+    target: any,
+    property: string | symbol,
+    method: "access" | "set"
+): any {
+    if (!Object.prototype.hasOwnProperty.call(target, property)) {
+        throw new Error(
+            `Attempted to ${method} property '${String(property)}', but it was not mocked.`
+        );
+    }
+    const value = target[property];
+    if (value && Object.prototype.hasOwnProperty.call(value, "_wasThrownByRealObject")) {
+        throw value;
+    }
+    return value;
+}
+
 /**
  * Creates a MockedObject from an interface or class. Converts any functions into SinonStubs.
  *
@@ -145,24 +164,12 @@ function replaceWithMocks<T>(obj: Partial<T>): MockedObject<T> {
  */
 export function mockObject<T>(overrides: Partial<T>): MockedObject<T> {
     const clonedObject = replaceWithMocks<T>(overrides);
-    function checkAndAcquireValueFromTarget(target: any, property: string | symbol): any {
-        if (!Object.prototype.hasOwnProperty.call(target, property)) {
-            throw new Error(
-                `Attempted to access property '${String(property)}', but it was not mocked.`
-            );
-        }
-        const value = target[property];
-        if (value && Object.prototype.hasOwnProperty.call(value, "_wasThrownByRealObject")) {
-            throw value;
-        }
-        return value;
-    }
     return new Proxy<any>(clonedObject, {
         get(target, property) {
-            return checkAndAcquireValueFromTarget(target, property);
+            return checkAndAcquireValueFromTarget(target, property, "access");
         },
         set(target, property, value) {
-            checkAndAcquireValueFromTarget(target, property);
+            checkAndAcquireValueFromTarget(target, property, "set");
             target[property] = value;
             return true;
         },
@@ -297,17 +304,18 @@ function shallowClone<T>(obj: T): T {
  *
  * **Note:** This **MUST** be called outside of the test() function or it will not work.
  *
- * @param mod The module that will be fully mocked
+ * @param module The module that will be fully mocked
+ * @param overrides Used to mock only certain properties within the module
  */
-export function mockGlobalModule<T>(mod: T): MockedObject<T> {
+export function mockGlobalModule<T>(module: T, overrides?: Partial<T>): MockedObject<T> {
     let realMock: MockedObject<T>;
-    const originalValue: T = shallowClone(mod);
+    const originalValue: T = shallowClone(module);
     // Create the mock at setup
     setup(() => {
-        realMock = mockObject(mod);
+        realMock = mockObject(overrides ?? module);
         for (const property of Object.getOwnPropertyNames(realMock)) {
             try {
-                Object.defineProperty(mod, property, {
+                Object.defineProperty(module, property, {
                     value: (realMock as any)[property],
                     writable: true,
                 });
@@ -318,9 +326,9 @@ export function mockGlobalModule<T>(mod: T): MockedObject<T> {
     });
     // Restore original value at teardown
     teardown(() => {
-        for (const property of Object.getOwnPropertyNames(originalValue)) {
+        for (const property of Object.getOwnPropertyNames(realMock)) {
             try {
-                Object.defineProperty(mod, property, {
+                Object.defineProperty(module, property, {
                     value: (originalValue as any)[property],
                 });
             } catch {
@@ -334,10 +342,15 @@ export function mockGlobalModule<T>(mod: T): MockedObject<T> {
             if (!realMock) {
                 throw Error("Mock proxy accessed before setup()");
             }
-            return (mod as any)[property];
+            checkAndAcquireValueFromTarget(realMock, property, "access");
+            return (module as any)[property];
         },
         set(target, property, value) {
-            (mod as any)[property] = value;
+            if (!realMock) {
+                throw Error("Mock proxy accessed before setup()");
+            }
+            checkAndAcquireValueFromTarget(realMock, property, "set");
+            (module as any)[property] = value;
             return true;
         },
     });
@@ -484,3 +497,16 @@ export class AsyncEventEmitter<T> {
         }
     }
 }
+
+/**
+ * Handles setup() and teardown() of the "mock-fs" module to temporarily mock out
+ * Node's "fs" module for the duration of a test.
+ *
+ * @param config the base configuration that will be set before each test
+ * @returns the "mock-fs" module for use in test cases
+ */
+export function mockFileSystem(config: FileSystem.DirectoryItems = {}): typeof mockFS {
+    setup(() => mockFS(config));
+    teardown(() => mockFS.restore());
+    return mockFS;
+}

test/unit-tests/MockUtils.test.ts

@@ -16,6 +16,7 @@ import { expect } from "chai";
 import { stub } from "sinon";
 import * as vscode from "vscode";
 import * as fs from "fs/promises";
+import * as os from "os";
 import {
     AsyncEventEmitter,
     mockFn,
@@ -207,41 +208,51 @@ suite("MockUtils Test Suite", () => {
 
     suite("mockGlobalModule()", () => {
         const mockedFS = mockGlobalModule(fs);
+        const mockedOS = mockGlobalModule(os, { homedir: () => "" });
         const mockedContextKeys = mockGlobalModule(contextKeys);
         const mockedConfiguration = mockGlobalModule(configuration);
 
-        test("can mock the fs/promises module", async () => {
+        test("replaces functions within the module with Sinon stubs", async () => {
             mockedFS.readFile.resolves("file contents");
 
             await expect(fs.readFile("some_file")).to.eventually.equal("file contents");
             expect(mockedFS.readFile).to.have.been.calledOnceWithExactly("some_file");
         });
 
-        test("can mock the contextKeys module", () => {
+        test("replaces properties within the module with writable values", () => {
             // Initial value should be undefined
             expect(contextKeys.isActivated).to.be.undefined;
+            expect(mockedContextKeys.isActivated).to.be.undefined;
             // Make sure that you can set the value of contextKeys using the mock
             mockedContextKeys.isActivated = true;
             expect(contextKeys.isActivated).to.be.true;
+            expect(mockedContextKeys.isActivated).to.be.true;
             // Make sure that setting isActivated via contextKeys is also possible
             contextKeys.isActivated = false;
             expect(contextKeys.isActivated).to.be.false;
+            expect(mockedContextKeys.isActivated).to.be.false;
         });
 
-        test("can mock the configuration module", () => {
-            expect(configuration.sdk).to.equal("");
-            // Make sure you can set a value using the mock
-            mockedConfiguration.sdk = "macOS";
-            expect(configuration.sdk).to.equal("macOS");
-            // Make sure you can set a value using the real module
-            configuration.sdk = "watchOS";
-            expect(configuration.sdk).to.equal("watchOS");
+        test("allows deeply nested mocks for the configuration module", () => {
             // Mocking objects within the configuration requires separate MockedObjects
             const mockedLspConfig = mockObject<(typeof configuration)["lsp"]>(configuration.lsp);
             mockedConfiguration.lsp = mockedLspConfig;
             mockedLspConfig.disable = true;
             expect(configuration.lsp.disable).to.be.true;
         });
+
+        test("can be used to mock only certain properties within a global module", () => {
+            mockedOS.homedir.returns("/path/to/my/custom/homedir");
+            expect(os.homedir()).to.equal("/path/to/my/custom/homedir");
+            // Make sure that accessing non-mocked properties of the mocked module fails
+            expect(() => mockedOS.arch.returns("arm64")).to.throw(
+                "Attempted to access property 'arch', but it was not mocked"
+            );
+            // Make sure that setting non-mocked properties on the mocked module fails
+            expect(() => (mockedOS.EOL = "\n")).to.throw(
+                "Attempted to set property 'EOL', but it was not mocked"
+            );
+        });
     });
 
     suite("mockGlobalValue()", () => {