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.

Sign up for GitHub

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

Interpret null Describe TestName to mean value can't be eval'd #1701

Merged
merged 2 commits into from
Jan 18, 2019
Merged

Interpret null Describe TestName to mean value can't be eval'd #1701

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f99901
Interpret null Describe TestName to mean value can't be eval'd
rkeithhill Jan 17, 2019
05bcbfb
Address PR feedback - more changes but not relying on null/undefined
rkeithhill Jan 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/features/PesterTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ export class PesterTestsFeature implements IFeature {
this.command = vscode.commands.registerCommand(
"PowerShell.RunPesterTestsFromFile",
() => {
this.launchTests(vscode.window.activeTextEditor.document.uri, false);
this.launchTests(vscode.window.activeTextEditor.document.uri, false, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The true, undefined and false, undefined are starting to look a bit hairy to my mind (i.e. in C# I'd look to use named parameters). Is there a way we could be more explicit about these parameters, like using TypeScript optional parameters or similar? More a question than a veiled suggestion.

Copy link
Contributor Author

@rkeithhill rkeithhill Jan 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the same thought crossed my mind as I was coding it up. It can obviously be made to work but feels brittle. I think we could handle this better - perhaps have a fourth parameter to the launch function which indicates the desire is to debug/run the whole file. Let me take a crack at this tonight.

});
this.command = vscode.commands.registerCommand(
"PowerShell.DebugPesterTestsFromFile",
() => {
this.launchTests(vscode.window.activeTextEditor.document.uri, true);
this.launchTests(vscode.window.activeTextEditor.document.uri, true, undefined);
});
// This command is provided for usage by PowerShellEditorServices (PSES) only
this.command = vscode.commands.registerCommand(
Expand All @@ -41,7 +41,21 @@ export class PesterTestsFeature implements IFeature {
this.languageClient = languageClient;
}

private launchTests(uriString, runInDebugger, describeBlockName?) {
private async launchTests(uriString, runInDebugger, describeBlockName?) {
// PSES passes null for the describeBlockName to signal that it can't evaluate the TestName.
if (describeBlockName === null) {
const answer = await vscode.window.showErrorMessage(
"This Describe block's TestName parameter cannot be evaluated. " +
`Would you like to ${runInDebugger ? "debug" : "run"} all the tests in this file?`,
"Yes", "No");

if (answer === "Yes") {
describeBlockName = undefined;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would make this:

if (answer === "No") {
    return;
}

describeBlockName = undefined;

but I recognise it's a style preference as much as anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is cleaner. :-) I'll make the change.

return;
}
}

const uri = vscode.Uri.parse(uriString);
const currentDocument = vscode.window.activeTextEditor.document;
const settings = Settings.load();
Expand Down