Interpret null Describe TestName to mean value can't be eval'd #1701
Conversation
|
This is the dialog that pops in this case:
|
src/features/PesterTests.ts
Outdated
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/features/PesterTests.ts
Outdated
|
|
||
| if (answer === "Yes") { | ||
| describeBlockName = undefined; | ||
| } else { |
There was a problem hiding this comment.
Personally I would make this:
if (answer === "No") {
return;
}
describeBlockName = undefined;but I recognise it's a style preference as much as anything.
There was a problem hiding this comment.
Yeah, that is cleaner. :-) I'll make the change.
|
@rjmholt @SydneyhSmith Any thoughts on the text in the dialog above and whether it should be an error dialog? Good enough? |
I think the dialog is perfect |
PR Summary
PSES will now return a null for
describeBlockNameto mean that this is a valid Pester Describe block but we couldn't evaluate the test name. This will cause the extension to pop a dialog indicating this and giving the user the chance to debug or run all the tests in the file.Two things to consider: 1) the text of the message in the dialog box and 2) should this be an error box (as it is now) or a warning box?
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready