Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix path regressions and cover with tests #3570

Merged
merged 13 commits into from
Sep 28, 2021
Merged

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Sep 15, 2021

This is hard to get right, and harder to test.

So the path on disk for all the code (because of bundling) is:

~/.vscode/extensions/ms-vscode.powershell-preview-2021.9.1/out/main.js

Hence, path.resolve(__dirname, "../") moves us to:

~/.vscode/extensions/ms-vscode.powershell-preview-2021.9.1

Because __dirname is the folder containing main.js (that is, out). And that's what has:

> ls ~/.vscode/extensions/ms-vscode.powershell-preview-2021.9.1
 CHANGELOG.md               docs       modules        test-results.xml
 LICENSE.txt                examples   out            themes
 README.md                  logs       package.json
'Third Party Notices.txt'   media      snippets

But wow, getting a test written to cover this is nigh-impossible due to the design.

When I bundled I was relying on successful compilation and tests, since there were a number of paths. I shouldn't have solely relied on that. I spent the better part of today trying to write a test to cover this, and took three different approaches just to check that the path used for OpenExamplesFolder is correct. But I cannot see how to test it sufficiently.

@andyleejordan
Copy link
Member Author

@TylerLeonhardt Pinged you on Teams, if you have any suggestions on how to assert what seemed like such a simple thing. But sadly the VS Code API does not give me meaningful return values for OpenExamplesFolder, and requireing the source code into the test code changes __dirname. All I want to do is have the test wait for the extension to be loaded, and then somehow (which I figured would be available through Code's API) check internals of the loaded extension.

@andyleejordan
Copy link
Member Author

WIP test file with some of those attemps:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

import * as assert from "assert";
import * as vscode from "vscode";
import { before, beforeEach, afterEach } from "mocha";
import { ExamplesFeature } from "../../src/features/Examples";

// tslint:disable-next-line: no-var-requires
const packageJSON: any = require("../../../package.json");
const extensionId = `${packageJSON.publisher}.${packageJSON.name}`;
let extension: any;

suite("Startup behavior", () => {
    before(async () => {
        extension = vscode.extensions.getExtension(extensionId);
        if (!extension.isActive) {
            await extension.activate();
        }
    });

    test("Should know where the examples are", () => {
        // const x = new ExamplesFeature() as any;
        // const path = x.examplesPath as string;
        // assert(path.endsWith(`extensions/${extensionId}/examples`));
        vscode.commands.getCommands()
        const x = extension.commandRegistrations[0] as any;
        const path = x.examplesPath as string;
        assert(path.endsWith(`extensions/${extensionId}/examples`));
    })


    // test("The examples folder can be opened", async () => {
    //     await vscode.commands.executeCommand("PowerShell.OpenExamplesFolder");
    // });
    // test("The session folder is created in the right place", async () => {
    // })
});

@andyleejordan
Copy link
Member Author

Hey @rjmholt and @JustinGrote look at that, regression tests!

@andyleejordan andyleejordan added the Issue-Bug A bug to squash. label Sep 16, 2021
@andyleejordan andyleejordan changed the title Fix relative paths (again) Fix path regressions and cover with tests Sep 16, 2021
@andyleejordan
Copy link
Member Author

Some strange CI issues with PowerShell 5.1:

  1) ISECompatibility feature
       It unsets ISE Settings:

      AssertionError [ERR_ASSERTION]: false != false
      + expected - actual


  	at d:\a\1\s\vscode-powershell\out\test\features\ISECompatibility.test.js:32:20
  	at Generator.next (<anonymous>)
  	at fulfilled (d:\a\1\s\vscode-powershell\out\test\features\ISECompatibility.test.js:7:58)
  	at processTicksAndRejections (internal/process/task_queues.js:93:5)


  2) ISECompatibility feature
       It leaves Theme after being changed after enabling ISE Mode:

      AssertionError [ERR_ASSERTION]: 'powershell' != 'powershell'
      + expected - actual


  	at d:\a\1\s\vscode-powershell\out\test\features\ISECompatibility.test.js:42:20
  	at Generator.next (<anonymous>)
  	at fulfilled (d:\a\1\s\vscode-powershell\out\test\features\ISECompatibility.test.js:7:58)
  	at processTicksAndRejections (internal/process/task_queues.js:93:5)


  3) RunCode tests
       Can run Pester tests from file:
     Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (d:\a\1\s\vscode-powershell\out\test\features\RunCode.test.js)
  	at listOnTimeout (internal/timers.js:554:17)
  	at processTimers (internal/timers.js:497:7)

@andyleejordan andyleejordan added this to In progress in Tests via automation Sep 21, 2021
@andyleejordan andyleejordan added this to the Committed-vNext milestone Sep 21, 2021
@andyleejordan
Copy link
Member Author

I think I fixed the CI issues. It was unfortunately due to Windows PowerShell bundling Pester 3.0.4 which is ancient.

@andyleejordan
Copy link
Member Author

@rjmholt I think that this test is demonstrating the very real problems of our debuggers' reliability.

@andyleejordan andyleejordan force-pushed the andschwa/fix-paths branch 6 times, most recently from 45bd1c0 to 4cd0baa Compare September 23, 2021 23:18
@andyleejordan
Copy link
Member Author

Going to rebase and see if this is still failing regularly...

This are hard to get right, and harder to test.
Cleans up some repetitive code and makes tests more stable.
The `before` and `after` etc. are for BDD (e.g. `describe` and `it`),
but we're using TDD (e.g. `suite` and `test`). I think they're just
aliases of each other, but let's be correct.
So that they're always run as expected, leaving the state clean for the
next test regardless of ordering.
Save and restore the user's theme, which was annoying when running the
tests via Code's debugger. Don't use the default theme "Dark+" so that
the setting is actually propogated.
@andyleejordan
Copy link
Member Author

Hmm...things are looking up today!

@andyleejordan andyleejordan marked this pull request as ready for review September 28, 2021 19:58
@andyleejordan
Copy link
Member Author

@rjmholt whacha think? This has passed four times in a row now, the inconsistency seems gone. I don't really know what changed except perhaps a better mitigation of the race condition through a forced (and rather long) sleep.

@andyleejordan
Copy link
Member Author

Repeating tests again for safety's sake!

@andyleejordan andyleejordan merged commit f0fddeb into master Sep 28, 2021
@andyleejordan andyleejordan deleted the andschwa/fix-paths branch September 28, 2021 20:54
Tests automation moved this from In progress to Done Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Test Issue-Bug A bug to squash.
Projects
No open projects
Tests
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants