Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix path regressions and cover with tests #3570

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

Merged
andyleejordan merged 13 commits into master from andschwa/fix-paths
Sep 28, 2021
Merged

Conversation

Copy link
Member

@andyleejordan andyleejordan commented Sep 15, 2021
edited
Loading

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.

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.

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 () => {
 // })
});

Copy link
Member Author

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

JustinGrote reacted with heart emoji

@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
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:\a1円\s\vscode-powershell\out\test\features\ISECompatibility.test.js:32:20
 	at Generator.next (<anonymous>)
 	at fulfilled (d:\a1円\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:\a1円\s\vscode-powershell\out\test\features\ISECompatibility.test.js:42:20
 	at Generator.next (<anonymous>)
 	at fulfilled (d:\a1円\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:\a1円\s\vscode-powershell\out\test\features\RunCode.test.js)
 	at listOnTimeout (internal/timers.js:554:17)
 	at processTimers (internal/timers.js:497:7)

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.

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
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.
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
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.

Copy link
Member Author

Repeating tests again for safety's sake!

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

@TylerLeonhardt TylerLeonhardt Awaiting requested review from TylerLeonhardt

1 more reviewer

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Area-Test Issue-Bug A bug to squash.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /