Clean up unit tests' descriptions and logical separation (#3641)

I read through all the tests in order to firstly understand fully what
we're covering and secondly be able to write correct BDD-style
descriptions. I split tests up that could be for more logical
separation, and formatted the files.

I also enabled tests that had been previously disabled/skipped
for reasons that are no longer applicable.

We now have 55 separate unit tests, up from 35.

Author: andyleejordan

test/core/paths.test.ts

@@ -7,21 +7,21 @@ import * as path from "path";
 import * as vscode from "vscode";
 import utils = require("../utils");
 
-describe("Path assumptions", function() {
+describe("Path assumptions", function () {
     before(utils.ensureExtensionIsActivated);
 
     // TODO: This is skipped because it intereferes with other tests. Either
     // need to find a way to close the opened folder via a Code API, or find
     // another way to test this.
-    it.skip("The examples folder can be opened (and exists)", async function() {
+    it.skip("Opens the examples folder at the expected path", async function () {
         assert(await vscode.commands.executeCommand("PowerShell.OpenExamplesFolder"));
     });
 
-    it("The session folder is created in the right place", async function() {
+    it("Creates the session folder at the correct path", function () {
         assert(fs.existsSync(path.resolve(utils.rootPath, "sessions")));
     });
 
-    it("The logs folder is created in the right place", async function() {
+    it("Creates the log folder at the correct path", function () {
         assert(fs.existsSync(path.resolve(utils.rootPath, "logs")));
     });
 });

test/core/platform.test.ts

@@ -467,8 +467,8 @@ function setupTestEnvironment(testPlatform: ITestPlatform) {
     }
 }
 
-describe("Platform module", function() {
-    describe("PlatformDetails", function() {
+describe("Platform module", function () {
+    it("Gets the correct platform details", function () {
         const platformDetails: platform.IPlatformDetails = platform.getPlatformDetails();
         switch (process.platform) {
             case "darwin":
@@ -517,18 +517,18 @@ describe("Platform module", function() {
                 return;
 
             default:
-                assert.fail("Tests run on unsupported platform");
+                assert.fail("This platform is unsupported");
         }
     });
 
-    describe("Default PowerShell installation", function() {
-        afterEach(function() {
+    describe("Default PowerShell installation", function () {
+        afterEach(function () {
             sinon.restore();
             mockFS.restore();
         });
 
         for (const testPlatform of successTestCases) {
-            it(`Default PowerShell path on ${testPlatform.name}`, function() {
+            it(`Finds it on ${testPlatform.name}`, function () {
                 setupTestEnvironment(testPlatform);
 
                 const powerShellExeFinder = new platform.PowerShellExeFinder(testPlatform.platformDetails);
@@ -542,7 +542,7 @@ describe("Platform module", function() {
         }
 
         for (const testPlatform of errorTestCases) {
-            it(`Extension startup fails gracefully on ${testPlatform.name}`, function() {
+            it(`Fails gracefully on ${testPlatform.name}`, function () {
                 setupTestEnvironment(testPlatform);
 
                 const powerShellExeFinder = new platform.PowerShellExeFinder(testPlatform.platformDetails);
@@ -553,14 +553,14 @@ describe("Platform module", function() {
         }
     });
 
-    describe("Expected PowerShell installation list", function() {
-        afterEach(function() {
+    describe("Expected PowerShell installation list", function () {
+        afterEach(function () {
             sinon.restore();
             mockFS.restore();
         });
 
         for (const testPlatform of successTestCases) {
-            it(`PowerShell installation list on ${testPlatform.name}`, function() {
+            it(`Finds them on ${testPlatform.name}`, function () {
                 setupTestEnvironment(testPlatform);
 
                 const powerShellExeFinder = new platform.PowerShellExeFinder(testPlatform.platformDetails);
@@ -583,7 +583,7 @@ describe("Platform module", function() {
         }
 
         for (const testPlatform of errorTestCases) {
-            it(`Extension startup fails gracefully on ${testPlatform.name}`, function() {
+            it(`Fails gracefully on ${testPlatform.name}`, function () {
                 setupTestEnvironment(testPlatform);
 
                 const powerShellExeFinder = new platform.PowerShellExeFinder(testPlatform.platformDetails);
@@ -594,16 +594,16 @@ describe("Platform module", function() {
         }
     });
 
-    describe("Windows PowerShell path fix", function() {
-        afterEach(function() {
+    describe("Windows PowerShell path fix", function () {
+        afterEach(function () {
             sinon.restore();
             mockFS.restore();
         });
 
         for (const testPlatform of successTestCases
-                .filter((tp) => tp.platformDetails.operatingSystem === platform.OperatingSystem.Windows)) {
+            .filter((tp) => tp.platformDetails.operatingSystem === platform.OperatingSystem.Windows)) {
 
-            it(`Corrects the Windows PowerShell path on ${testPlatform.name}`, function() {
+            it(`Corrects the Windows PowerShell path on ${testPlatform.name}`, function () {
                 setupTestEnvironment(testPlatform);
 
                 function getWinPSPath(systemDir: string) {

test/core/settings.test.ts

@@ -5,36 +5,38 @@ import * as assert from "assert";
 import * as vscode from "vscode";
 import Settings = require("../../src/settings");
 
-describe("Settings module", function() {
-    it("Settings load without error", function() {
+describe("Settings module", function () {
+    it("Loads without error", function () {
         assert.doesNotThrow(Settings.load);
     });
 
-    it("Settings update correctly", async function() {
-        // then syntax
+    it("Updates correctly with 'then' syntax", async function () {
         Settings.change("helpCompletion", "BlockComment", false).then(() =>
             assert.strictEqual(Settings.load().helpCompletion, "BlockComment"));
+    });
 
-        // async/await syntax
+    it("Updates correctly with 'async/await' syntax", async function () {
         await Settings.change("helpCompletion", "LineComment", false);
         assert.strictEqual(Settings.load().helpCompletion, "LineComment");
     });
 
-    it("Settings that can only be user settings update correctly", async function() {
-        // set to false means it's set as a workspace-level setting so this should throw.
+    describe("User-only settings", async function () {
         const psExeDetails = [{
             versionName: "My PowerShell",
             exePath: "dummyPath",
         }];
 
-        assert.rejects(async () => await Settings.change("powerShellAdditionalExePaths", psExeDetails, false));
+        it("Throws when updating at workspace-level", async function () {
+            assert.rejects(async () => await Settings.change("powerShellAdditionalExePaths", psExeDetails, false /* workspace-level */));
+        });
 
-        // set to true means it's a user-level setting so this should not throw.
-        await Settings.change("powerShellAdditionalExePaths", psExeDetails, true);
-        assert.strictEqual(Settings.load().powerShellAdditionalExePaths[0].versionName, psExeDetails[0].versionName);
+        it("Doesn't throw when updating at user-level", async function () {
+            await Settings.change("powerShellAdditionalExePaths", psExeDetails, true /* user-level */);
+            assert.strictEqual(Settings.load().powerShellAdditionalExePaths[0].versionName, psExeDetails[0].versionName);
+        });
     });
 
-    it("Can get effective configuration target", async function() {
+    it("Gets the effective configuration target", async function () {
         await Settings.change("helpCompletion", "LineComment", false);
         let target = await Settings.getEffectiveConfigurationTarget("helpCompletion");
         assert.strictEqual(target, vscode.ConfigurationTarget.Workspace);

test/features/CustomViews.test.ts

@@ -30,11 +30,10 @@ function convertToVSCodeResourceScheme(filePath: string): string {
     return vscode.Uri.file(filePath).toString().replace("file://", "vscode-resource://");
 }
 
-describe("CustomViews tests", function() {
+describe("CustomViews feature", function () {
     const testCases: IHtmlContentViewTestCase[] = [
-        // Basic test that has no js or css.
         {
-            name: "Basic",
+            name: "with no JavaScript or CSS",
             htmlContent: "hello",
             javaScriptFiles: [],
             cssFiles: [],
@@ -45,7 +44,7 @@ hello
 
         // A test that adds a js file.
         {
-            name: "With JavaScript file",
+            name: "with a JavaScript file but no CSS",
             htmlContent: "hello",
             javaScriptFiles: [
                 {
@@ -62,7 +61,7 @@ hello
 
         // A test that adds a js file in the current directory, and the parent directory.
         {
-            name: "With 2 JavaScript files in two different locations",
+            name: "with two JavaScript files in different locations, but no CSS",
             htmlContent: "hello",
             javaScriptFiles: [
                 {
@@ -84,7 +83,7 @@ hello
 
         // A test that adds a js file and a css file.
         {
-            name: "With JavaScript and CSS file",
+            name: "with a JavaScript and a CSS file",
             htmlContent: "hello",
             javaScriptFiles: [
                 {
@@ -98,8 +97,7 @@ hello
                     content: "body: { background-color: green; }",
                 },
             ],
-            expectedHtmlString: `<html><head><link rel="stylesheet" href="${
-                convertToVSCodeResourceScheme(path.join(__dirname, "testCustomViews.css"))}">
+            expectedHtmlString: `<html><head><link rel="stylesheet" href="${convertToVSCodeResourceScheme(path.join(__dirname, "testCustomViews.css"))}">
 </head><body>
 hello
 <script src="${convertToVSCodeResourceScheme(path.join(__dirname, "testCustomViews.js"))}"></script>
@@ -108,7 +106,7 @@ hello
     ];
 
     for (const testCase of testCases) {
-        it(`Can create an HtmlContentView and get its content - ${testCase.name}`, function() {
+        it(`Correctly creates an HtmlContentView ${testCase.name}`, function () {
             const htmlContentView = new HtmlContentView();
 
             const jsPaths = testCase.javaScriptFiles.map((jsFile) => {

test/features/ExternalApi.test.ts

@@ -5,93 +5,85 @@ import * as assert from "assert";
 import utils = require("../utils");
 import { IExternalPowerShellDetails, IPowerShellExtensionClient } from "../../src/features/ExternalApi";
 
-describe("ExternalApi feature - Registration API", function() {
-    let powerShellExtensionClient: IPowerShellExtensionClient;
-    before(async function() {
-        const powershellExtension = await utils.ensureExtensionIsActivated();
-        powerShellExtensionClient = powershellExtension!.exports as IPowerShellExtensionClient;
-    });
+describe("ExternalApi feature", function () {
+    describe("External extension registration", function () {
+        let powerShellExtensionClient: IPowerShellExtensionClient;
+        before(async function () {
+            const powershellExtension = await utils.ensureExtensionIsActivated();
+            powerShellExtensionClient = powershellExtension!.exports as IPowerShellExtensionClient;
+        });
 
-    it("It can register and unregister an extension", function() {
-        const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
-        assert.notStrictEqual(sessionId , "");
-        assert.notStrictEqual(sessionId , null);
-        assert.strictEqual(
-            powerShellExtensionClient.unregisterExternalExtension(sessionId),
-            true);
-    });
+        it("Registers and unregisters an extension", function () {
+            const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
+            assert.notStrictEqual(sessionId, "");
+            assert.notStrictEqual(sessionId, null);
+            assert.strictEqual(
+                powerShellExtensionClient.unregisterExternalExtension(sessionId),
+                true);
+        });
 
-    it("It can register and unregister an extension with a version", function() {
-        const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId, "v2");
-        assert.notStrictEqual(sessionId , "");
-        assert.notStrictEqual(sessionId , null);
-        assert.strictEqual(
-            powerShellExtensionClient.unregisterExternalExtension(sessionId),
-            true);
-    });
+        it("Registers and unregisters an extension with a version", function () {
+            const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId, "v2");
+            assert.notStrictEqual(sessionId, "");
+            assert.notStrictEqual(sessionId, null);
+            assert.strictEqual(
+                powerShellExtensionClient.unregisterExternalExtension(sessionId),
+                true);
+        });
 
-    /*
-        NEGATIVE TESTS
-    */
-    it("API fails if not registered", async function() {
-        assert.rejects(
-            async () => await powerShellExtensionClient.getPowerShellVersionDetails(""))
-    });
+        it("Rejects if not registered", async function () {
+            assert.rejects(
+                async () => await powerShellExtensionClient.getPowerShellVersionDetails(""))
+        });
+
+        it("Throws if attempting to register an extension more than once", async function () {
+            const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
+            try {
+                assert.throws(
+                    () => powerShellExtensionClient.registerExternalExtension(utils.extensionId),
+                    {
+                        message: `The extension '${utils.extensionId}' is already registered.`
+                    });
+            } finally {
+                powerShellExtensionClient.unregisterExternalExtension(sessionId);
+            }
+        });
 
-    it("It can't register the same extension twice", async function() {
-        const sessionId: string = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
-        try {
+        it("Throws when unregistering an extension that isn't registered", async function () {
             assert.throws(
-                () => powerShellExtensionClient.registerExternalExtension(utils.extensionId),
+                () => powerShellExtensionClient.unregisterExternalExtension("not-real"),
                 {
-                    message: `The extension '${utils.extensionId}' is already registered.`
+                    message: `No extension registered with session UUID: not-real`
                 });
-        } finally {
-            powerShellExtensionClient.unregisterExternalExtension(sessionId);
-        }
-    });
-
-    it("It can't unregister an extension that isn't registered", async function() {
-        assert.throws(
-            () => powerShellExtensionClient.unregisterExternalExtension("not-real"),
-            {
-                message: `No extension registered with session UUID: not-real`
-            });
         });
-});
-
-describe("ExternalApi feature - Other APIs", () => {
-    let sessionId: string;
-    let powerShellExtensionClient: IPowerShellExtensionClient;
-
-    before(async function() {
-        const powershellExtension = await utils.ensureExtensionIsActivated();
-        powerShellExtensionClient = powershellExtension!.exports as IPowerShellExtensionClient;
     });
 
-    beforeEach(function() {
-        sessionId = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
-    });
+    describe("PowerShell version details", () => {
+        let sessionId: string;
+        let powerShellExtensionClient: IPowerShellExtensionClient;
 
-    afterEach(function() {
-        powerShellExtensionClient.unregisterExternalExtension(sessionId);
-    });
+        before(async function () {
+            const powershellExtension = await utils.ensureExtensionIsActivated();
+            powerShellExtensionClient = powershellExtension!.exports as IPowerShellExtensionClient;
+            sessionId = powerShellExtensionClient.registerExternalExtension(utils.extensionId);
+        });
 
-    it("It can get PowerShell version details", async function() {
-        const versionDetails: IExternalPowerShellDetails = await powerShellExtensionClient.getPowerShellVersionDetails(sessionId);
+        after(function () { powerShellExtensionClient.unregisterExternalExtension(sessionId); });
 
-        assert.notStrictEqual(versionDetails.architecture, "");
-        assert.notStrictEqual(versionDetails.architecture, null);
+        it("Gets non-empty version details from the PowerShell Editor Services", async function () {
+            const versionDetails: IExternalPowerShellDetails = await powerShellExtensionClient.getPowerShellVersionDetails(sessionId);
 
-        assert.notStrictEqual(versionDetails.displayName, "");
-        assert.notStrictEqual(versionDetails.displayName, null);
+            assert.notStrictEqual(versionDetails.architecture, "");
+            assert.notStrictEqual(versionDetails.architecture, null);
 
-        assert.notStrictEqual(versionDetails.exePath, "");
-        assert.notStrictEqual(versionDetails.exePath, null);
+            assert.notStrictEqual(versionDetails.displayName, "");
+            assert.notStrictEqual(versionDetails.displayName, null);
 
-        assert.notStrictEqual(versionDetails.version, "");
-        assert.notStrictEqual(versionDetails.version, null);
+            assert.notStrictEqual(versionDetails.exePath, "");
+            assert.notStrictEqual(versionDetails.exePath, null);
 
-        // Start up can take some time...so set the timeout to 30 seconds.
+            assert.notStrictEqual(versionDetails.version, "");
+            assert.notStrictEqual(versionDetails.version, null);
+        });
     });
 });