-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Test Case Recorder #87
Changes from 25 commits
f7587e3
815e829
ed4ee0d
3d0e8b0
e9b60e4
efb696c
e95a993
b06f27f
c51def0
cc0e4bf
9d65fde
fa52794
35a8ce5
0471c7f
09a743f
43a4473
829882d
087c7fd
50ff1e4
f6db9e4
be29b40
84326b4
c6cd5ec
ca334b2
01b8553
c5c3d9b
060d91d
08ebc6b
076da28
52f4408
ff6a1e1
f585de0
b8df923
decc22f
16f67bb
0b8df41
3f188a3
a255e0b
8ce8f86
39d62f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import * as vscode from "vscode"; | ||
|
||
/** | ||
* A mockable layer over the vscode clipboard | ||
* | ||
* For unknown reasons it's not possible to mock the clipboard directly. | ||
* Use this instead of vscode.env.clipboard so it can be mocked in testing. | ||
**/ | ||
export class Clipboard { | ||
static readText = vscode.env.clipboard.readText; | ||
static writeText = vscode.env.clipboard.writeText; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
import * as path from "path"; | ||
import * as fs from "fs"; | ||
import * as yaml from "js-yaml"; | ||
import * as vscode from "vscode"; | ||
import NavigationMap from "./NavigationMap"; | ||
import { ThatMark } from "./ThatMark"; | ||
import { ActionType, PartialTarget, Target } from "./Types"; | ||
import { extractTargetedMarks } from "./extractTargetedMarks"; | ||
import { serializeMarks, SerializedMarks } from "./serializers"; | ||
import { takeSnapshot, TestCaseSnapshot } from "./takeSnapshot"; | ||
|
||
type TestCaseCommand = { | ||
actionName: ActionType; | ||
partialTargets: PartialTarget[]; | ||
extraArgs: any[]; | ||
}; | ||
|
||
type TestCaseContext = { | ||
talonCommand: string; | ||
thatMark: ThatMark; | ||
targets: Target[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a funny smell to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I follow. Do you mean you would rather construct test case with a single argument/type ex. |
||
navigationMap: NavigationMap; | ||
}; | ||
|
||
export type TestCaseFixture = { | ||
talonCommand: string; | ||
command: TestCaseCommand; | ||
targets: Target[]; | ||
languageId: string; | ||
marks: SerializedMarks; | ||
initialState: TestCaseSnapshot; | ||
finalState: TestCaseSnapshot; | ||
returnValue: unknown; | ||
}; | ||
|
||
export class TestCase { | ||
talonCommand: string; | ||
command: TestCaseCommand; | ||
languageId: string; | ||
targets: Target[]; | ||
marks: SerializedMarks; | ||
context: TestCaseContext; | ||
initialState: TestCaseSnapshot | null = null; | ||
finalState: TestCaseSnapshot | null = null; | ||
returnValue: unknown = null; | ||
|
||
constructor(command: TestCaseCommand, context: TestCaseContext) { | ||
const activeEditor = vscode.window.activeTextEditor!; | ||
const { navigationMap, targets, talonCommand } = context; | ||
const targetedMarks = extractTargetedMarks(targets, navigationMap); | ||
|
||
this.talonCommand = talonCommand; | ||
this.command = command; | ||
this.languageId = activeEditor.document.languageId; | ||
this.marks = serializeMarks(targetedMarks); | ||
this.targets = targets; | ||
this.context = context; | ||
} | ||
|
||
private includesThatMark(target: Target) { | ||
if (target.type === "primitive" && target.mark.type === "that") { | ||
return true; | ||
} else if (target.type === "list") { | ||
return target.elements.some(this.includesThatMark, this); | ||
} else if (target.type === "range") { | ||
return [target.start, target.end].some(this.includesThatMark, this); | ||
} | ||
return false; | ||
} | ||
|
||
private getExcludedFields() { | ||
const excludableFields = { | ||
clipboard: !["copy", "paste"].includes(this.command.actionName), | ||
thatMark: | ||
this.initialState == null && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not immediately obvious to me why we check whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah looking at this I have to think about it too, maybe I can clarify the code. If we are taking the first snapshot, then we only want to record There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I figured that out later; I'd just pass something into the function? |
||
!this.targets.some(this.includesThatMark, this), | ||
visibleRanges: ![ | ||
"fold", | ||
"unfold", | ||
"scrollToBottom", | ||
"scrollToCenter", | ||
"scrollToTop", | ||
].includes(this.command.actionName), | ||
}; | ||
|
||
return Object.keys(excludableFields).filter( | ||
(field) => excludableFields[field] | ||
); | ||
} | ||
|
||
private toYaml() { | ||
if (this.initialState == null || this.finalState == null) { | ||
throw Error("Two snapshots must be taken before serializing"); | ||
} | ||
const fixture: TestCaseFixture = { | ||
talonCommand: this.talonCommand, | ||
languageId: this.languageId, | ||
command: this.command, | ||
targets: this.targets, | ||
marks: this.marks, | ||
initialState: this.initialState, | ||
finalState: this.finalState, | ||
returnValue: this.returnValue, | ||
}; | ||
return yaml.dump(fixture, { noRefs: true, quotingType: '"' }); | ||
} | ||
|
||
async recordInitialState() { | ||
const excludeFields = this.getExcludedFields(); | ||
this.initialState = await takeSnapshot( | ||
this.context.thatMark, | ||
excludeFields | ||
); | ||
} | ||
|
||
async recordFinalState(returnValue: unknown) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's basically like a type safe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL thanks |
||
const excludeFields = this.getExcludedFields(); | ||
this.returnValue = returnValue; | ||
this.finalState = await takeSnapshot(this.context.thatMark, excludeFields); | ||
} | ||
|
||
async writeFile(filename: string) { | ||
const fixture = this.toYaml(); | ||
const workspacePath = vscode.workspace.workspaceFolders?.[0].uri.path; | ||
let document; | ||
|
||
if (workspacePath && filename) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So are you assuming that the user is recording in the cursorless workspace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, wasn't really sure how to handle this. I wasn't assuming we were in the cursorless workspace, and that's why I moved fixtures from Now that I'm thinking about it, I could actually check if we're in the cursorless workspace or not. If we're not in the cursorless workspace, just show the yaml in a pane? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds good to me Btw I'm happy to call this out of scope but from a usability standpoint I think my ideal sequence would be:
Note that steps 3–5 only happen if in cursorless workspace So then what we'd do is to have a command in talon "record", which will grab the parsed command, call record on cursorless, type in literal command, then issue command. So workflow is then:
Should be able to rapidly record dozens of tests the way. Alternately, we could keep things as they are extension side but ask for dir and name before asking for talon command. Then we'd have two record commands. The first one is eg "setup recording" and the second is "record". So you'd say
Maybe the second approach is better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds good to me Btw I'm happy to call this out of scope but from a usability standpoint I think my ideal sequence would be:
Note that steps 3–5 only happen if in cursorless workspace So then what we'd do is to have a command in talon "record", which will grab the parsed command, call record on cursorless, type in literal command, then issue command. So workflow is then:
Should be able to rapidly record dozens of tests the way. Alternately, we could keep things as they are extension side but ask for dir and name before asking for talon command. Then we'd have two record commands. The first one is eg "setup recording" and the second is "record". So you'd say
Maybe the second approach is better? |
||
const fixturePath = path.join( | ||
workspacePath, | ||
"testFixtures", | ||
`${filename}.yml` | ||
); | ||
fs.writeFileSync(fixturePath, fixture); | ||
document = await vscode.workspace.openTextDocument(fixturePath); | ||
} else { | ||
document = await vscode.workspace.openTextDocument({ | ||
language: "yaml", | ||
content: fixture, | ||
}); | ||
} | ||
await vscode.window.showTextDocument(document, { | ||
viewColumn: vscode.ViewColumn.Beside, | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { SelectionWithEditor } from "./Types"; | ||
|
||
export class ThatMark { | ||
private mark: SelectionWithEditor[] = []; | ||
|
||
set(value: SelectionWithEditor[]) { | ||
this.mark = value; | ||
} | ||
|
||
get() { | ||
return this.mark; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness I'd be tempted to also capture the full target, even if we don't end up using it for anything