-
-
Notifications
You must be signed in to change notification settings - Fork 84
Test Case Recorder #87
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
Conversation
Great! Would be super helpful to paste an example snapshot yaml into a comment to give a sense of what it looks like |
Sure thing! command:
actionName: swap
partialTargets:
- type: primitive
mark:
type: decoratedSymbol
symbolColor: default
character: h
- type: primitive
mark:
type: decoratedSymbol
symbolColor: default
character: p
extraArgs: []
languageId: typescript
decorations:
default.h:
start:
line: 1
character: 15
end:
line: 1
character: 20
default.p:
start:
line: 4
character: 12
end:
line: 4
character: 17
initialState:
document: |
function helloWorld(name: string) {
console.log(`hello, ${name}`)
}
helloWorld("pokey")
selections:
- active: &ref_0
_line: 5
_character: 0
anchor: &ref_1
_line: 5
_character: 0
visibleRanges:
- start:
line: 0
character: 0
end:
line: 5
character: 0
finalState:
document: |
function helloWorld(name: string) {
console.log(`pokey, ${name}`)
}
helloWorld("hello")
selections:
- active: *ref_0
anchor: *ref_1
visibleRanges:
- start:
line: 0
character: 0
end:
line: 5
character: 0 And I already see that I should probably look into serializing values instead of the references for snapshot selections 🧐 |
Dude. This. Is. Epic. 😍 |
Looks great in yaml. Love how the it uses that block for the doc text. Super readable |
Gonna save us so much time writing test cases |
5d7c5ae
to
12098d6
Compare
Now able to run some test cases. Plenty to improve here, including the fact that cases with inferred targets seem to throw an error... For example, function transformSelection(
context: ProcessedTargetsContext,
target: PrimitiveTarget,
selection: SelectionWithEditor
): { selection: SelectionWithEditor; context: SelectionContext }[] {
const { transformation } = target;
switch (transformation.type) {
case "identity":
return [{ selection, context: {} }];
case "containingScope":
var node: SyntaxNode | null = context.getNodeAtLocation( // !!!
new vscode.Location(selection.editor.document.uri, selection.selection)
); throws on this fixture 🤔 I'm a little stumped at the moment. command:
actionName: swap
partialTargets:
- type: primitive
transformation:
type: containingScope
scopeType: namedFunction
- type: primitive
transformation:
type: containingScope
scopeType: ifStatement
mark:
type: decoratedSymbol
symbolColor: blue
character: i
extraArgs: []
languageId: python
decorations:
blue.i:
start:
line: 5
character: 0
end:
line: 5
character: 2
initialState:
document: |
import sys
def hello_world(name="pokey"):
print(f"hello, {name}")
if __name__ == "__main__":
hello_world(sys.argv[0])
selections:
- active:
line: 2
character: 20
anchor:
line: 2
character: 20
visibleRanges:
- start:
line: 0
character: 0
end:
line: 7
character: 0
clipboard: ""
finalState:
document: |
import sys
if __name__ == "__main__":
hello_world(sys.argv[0])
def hello_world(name="pokey"):
print(f"hello, {name}")
selections:
- active:
line: 2
character: 20
anchor:
line: 2
character: 20
visibleRanges:
- start:
line: 0
character: 0
end:
line: 7
character: 0
clipboard: "" |
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.
Nice!!
For that test that's not working, it's possible that the rootNode
can't be found because the syntax tree extension is not getting a chance to create a syntax tree? Easiest thing would be to try an async sleep (eg await sleep(500)
) right after you open the document. You might also check what that URI is that gets passed into the Location
. Problem could be that it's an "anonymous" doc. Might see if the way of loading up the fixture from my link in comment works any better
src/test/suite/recorded.test.ts
Outdated
fixture.initialState.selections.map(deserializeSelection); | ||
|
||
// TODO restore visible ranges? | ||
// Not sure of a straightforward way to do this. Maybe just use to test folding? |
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.
Used to test folding and scrolling; we can leave it for now
src/TestCase.ts
Outdated
document: activeEditor.document.getText(), | ||
selections: activeEditor.selections.map(serializeSelection), | ||
visibleRanges: activeEditor.visibleRanges.map(serializeRange), | ||
clipboard: await vscode.env.clipboard.readText(), |
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.
I'd be tempted to only bother with the clipboard if it's a copy or paste command
src/TestCase.ts
Outdated
const keys: string[] = []; | ||
targets.forEach((target) => { | ||
if (target.mark.type === "decoratedSymbol") { | ||
keys.push(`${target.mark.symbolColor}.${target.mark.character}`); |
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.
I would try to use the function in navigation map for this so that we don't duplicate the key generation here
|
||
type Command = { | ||
actionName: ActionType; | ||
partialTargets: PartialTarget[]; |
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
src/TestCase.ts
Outdated
return { line: position.line, character: position.character }; | ||
} | ||
|
||
type Command = { |
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.
It would be cool to capture the talon command. I wonder if we could use something like keeper to have talon automatically send it over. Tho fwiw I think keeper may be broken with recent talon? Been getting error messages with it
@@ -8,7 +8,7 @@ import processTargets from "../../processTargets"; | |||
import { Target, Token } from "../../Types"; | |||
// import * as myExtension from '../../extension'; | |||
|
|||
suite("processTargets", () => { | |||
suite.skip("processTargets", () => { |
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.
Here too
__dirname, | ||
// TODO What's the best way to handle this? | ||
"../../../src/test/suite/recordings" | ||
); |
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.
not saying this is better or worse than what you have here, but here's one approach that I had bookmarked in case helpful
src/test/suite/recorded.test.ts
Outdated
// TODO restore visible ranges? | ||
// Not sure of a straightforward way to do this. Maybe just use to test folding? | ||
|
||
const navigationMap = new NavigationMap(); |
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.
See above comment; if we can get away with it it may be worth just asserting that the map matches for the tokens we care about and then hopefully the assert doesn't fail
src/test/suite/recorded.test.ts
Outdated
editor: vscode.window.activeTextEditor!, | ||
}; | ||
const [color, character] = key.split("."); | ||
// @ts-ignore TODO should probably add a decoration color type? |
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.
not sure I follow; there is a token decoration color type
src/test/suite/recorded.test.ts
Outdated
}); | ||
const editor = await vscode.window.showTextDocument(document); | ||
|
||
await vscode.env.clipboard.writeText(fixture.initialState.clipboard); |
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.
I'd prefer to use a mock for this, so that we don't overwrite the real clipboard
Thanks @pokey, exactly what I needed to keep this going! |
Awesome! btw feel free to ping me on slack if you'd like when you post something in case I miss the email 😊. Always excited to see progress |
12098d6
to
09a743f
Compare
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.
Alright cool stuff! Left a bunch of comments. As mentioned below something feels slightly off about the TestCase
recording setup, but tough to tell with the file so big so let's extract some pieces to separate files and have another look. I think it's getting close; the meat of the code looks really good it's just some top-level restructuring that might need to happen which should be quick using vscode's refactoring tools
src/TestCase.ts
Outdated
return targetedDecorations; | ||
} | ||
|
||
isThatMarkTargeted() { |
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.
This won't work if a "that" mark is in a compound target, right? I think you need to descend like you do for finding decorated marks
src/NavigationMap.ts
Outdated
public serializeRanges() { | ||
const rangeMap: { [coloredSymbol: string]: SerializedRange } = {}; | ||
Object.entries(this.map).forEach(([key, value]) => { | ||
rangeMap[key] = serializeRange(value.range); | ||
}); | ||
return rangeMap; | ||
} |
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.
What is this for? Don't you just serialize the targets that actually get referred to?
src/TestCase.ts
Outdated
}; | ||
} | ||
|
||
async getSnapshot(): Promise<TestCaseSnapshot> { |
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.
I think it's a bit confusing to have both this function and the static one
src/TestCase.ts
Outdated
returnValue: any; | ||
}; | ||
|
||
export default class TestCase { |
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.
Something doesn't quite feel right here but it's tough to see everything with all the functions in this file. Maybe let's split things up a bit to get a better feel?
src/TestCase.ts
Outdated
this.context = context; | ||
} | ||
|
||
extractPrimitiveTargetKeys(...targets: PrimitiveTarget[]) { |
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.
I'd get all these primitive target extraction functions into another file
src/test/suite/recorded.test.ts
Outdated
assert.deepStrictEqual(fixture.finalState, resultState); | ||
assert.deepStrictEqual(fixture.returnValue, returnValue); | ||
|
||
sinon.restore(); |
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.
Per above, let's put this in a teardown
func
src/test/suite/recorded.test.ts
Outdated
cursorlessApi.thatMark.get() | ||
); | ||
assert.deepStrictEqual(fixture.finalState, resultState); | ||
assert.deepStrictEqual(fixture.returnValue, returnValue); |
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.
As mentioned above, we'll not include properties in the fixture that aren't relevant, eg clipboard, so we can avoid checking them if not necessary
src/test/suite/recorded.test.ts
Outdated
} | ||
|
||
let clip = fixture.initialState.clipboard; | ||
sinon.replace(Clipboard, "readText", async () => clip); |
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.
maybe only mock if clipboard is in fixture
testFixtures/chuckThat.yml
Outdated
thatMark: | ||
- active: | ||
line: 2 | ||
character: 0 | ||
anchor: | ||
line: 2 | ||
character: 0 |
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.
Is this the correct thatMark
? This test didn't work for me
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.
🤔 it is, I just checked.
testFixtures/swap.yml
Outdated
anchor: | ||
line: 7 | ||
character: 0 | ||
visibleRanges: [] |
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.
visible ranges broke for me here, but as mentioned we can just leave this property out and not check for it if it's not there
also, we should prob raise an exception in setup if they include visible ranges, because I don't think we properly support it yet
Don't define irrelevant fixture properties Extract test case helpers Remove navigation map serializer Clarify test case methods
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.
This is beautiful. Gotten super crisp. I left a dozen minor nits. I'm happy to knock them out but if you get to them first feel free to take them down
src/TestCase.ts
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Not immediately obvious to me why we check whether initialState
is null
here
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.
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 thatMark
when it is targeted. If we're taking the final snapshot, we always want to record thatMark
.
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.
Yeah I figured that out later; I'd just pass something into the function?
); | ||
} | ||
|
||
async recordFinalState(returnValue: unknown) { |
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.
Why unknown
instead of any
here? Tbh I don't really know the difference 😅
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.
It's basically like a type safe any
. You can use unknown
in the same places you would use any
, but it requires you to assert a type if you want to do anything with it.
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.
TIL thanks
src/TestCase.ts
Outdated
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 comment
The 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 comment
The 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 src/test/suite/fixtures/recorded
to a folder in the root.
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 comment
The 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:
- Ask user for talon command
- Wait for them to run command
- Have subfolders in the fixture directory and present them with a list of possible subdirectories and allow them to select "add new" if they type something that doesn't exist
- Ask them for a file name
- Display a message that the fixture was made, with a link to open it, but don't open it by default
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:
- Open a file to test on. We might consider storing a few common ones somewhere
- Say "record take air"
- Select subdirectory
- Say test name
- Reset file and repeat
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
- "setup recording in named ", which would call extension record and fill out dir and name
- "record take air", which would type in "take air" literally, then issue the command
Maybe the second approach is better?
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.
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:
- Ask user for talon command
- Wait for them to run command
- Have subfolders in the fixture directory and present them with a list of possible subdirectories and allow them to select "add new" if they type something that doesn't exist
- Ask them for a file name
- Display a message that the fixture was made, with a link to open it, but don't open it by default
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:
- Open a file to test on. We might consider storing a few common ones somewhere
- Say "record take air"
- Select subdirectory
- Say test name
- Reset file and repeat
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
- "setup recording in named ", which would call extension record and fill out dir and name
- "record take air", which would type in "take air" literally, then issue the command
Maybe the second approach is better?
src/extension.ts
Outdated
testCaseRecorder.filename = filename ?? ""; | ||
testCaseRecorder.talonCommand = talonCommand ?? ""; |
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.
I'd be tempted to use null
/ undefined
if they don't answer, but prob not a big deal
src/extractTargetedMarks.ts
Outdated
if (!navigationMap) { | ||
return {}; | ||
} |
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.
When does this happen?
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.
I don't think it actually does, but that's how it's typed due to the way the extension sets up on activation.
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.
Hmm but the type signature of function doesn't allow undefined or null right?
src/test/suite/recorded.test.ts
Outdated
} | ||
|
||
// Wait for cursorless to set up decorations | ||
await new Promise((resolve) => setTimeout(resolve, 300)); |
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.
Hmm. We might wanna figure out how to remove this delay when we get 50+ test cases but prob fine for now
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.
Yeah, I haven't played with the timeout, it could probably be lower. Maybe we add a function/property to see if decorations are finished that we can check periodically?
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.
Let's leave it for now. They run pretty quick. If it becomes a problem we can do something fancier
src/test/suite/recorded.test.ts
Outdated
|
||
// Assert that recorded decorations are present | ||
Object.entries(fixture.marks).forEach(([key, _]) => { | ||
const [color, character] = key.split(".") as [SymbolColor, string]; |
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.
As above, I'd let navigationMap
own this logic
src/test/suite/recorded.test.ts
Outdated
Object.entries(fixture.marks).forEach(([key, _]) => { | ||
const [color, character] = key.split(".") as [SymbolColor, string]; | ||
const token = cursorlessApi.navigationMap.getToken(color, character); | ||
assert(token != null, `Mark "${color} ${character}" not found`); |
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.
We should assert that this token (serialised) is equal to the serialised token, no?
src/test/suite/recorded.test.ts
Outdated
...fixture.command.extraArgs | ||
); | ||
|
||
// Do not assert visible ranges; for now they are included as context |
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.
We should prob be a bit louder about this and link an issue
testFixtures/chuckThat.yml
Outdated
type: that | ||
extraArgs: [] | ||
languageId: python | ||
targets: |
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.
I think I might call this fullTargets
or inferredFullTargets
? We might add a docstring to that field as well to indicate it's just for why-nots right now. Also maybe let's move this one to the end; looks like the order gets preserved when dumping
Tbh I'm tempted to remove it because it adds a big chunk to an otherwise quite compact test case format, but I'm very much on the fence
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.
Added a comment and moved it to the end. Should be easy to remove if we decide it's not worth keeping.
And one more minor one: for location of the yaml files, I'd be inclined to use |
I hit most of these. I'll see about the recording flow and fixture location later today! |
f278cc8
to
decc22f
Compare
@pokey Moved the fixtures and took a shot at the recording flow you described:
|
Nice! Will have one last look tomorrow and get it merged in Fwiw I am planning to make the following quick changes tomorrow before merging:
My goal is to have:
I'm planning to sit down for a bit tomorrow and will try to knock these out myself. But if I don't, I think I may just file some of them and merge in this PR without. If you get a chance before I do feel free to take a swing but I'll hopefully get a good chunk of time tomorrow |
May I suggest one improvement? Using
Here is my preliminary implementation |
This is the test case recorder for #59
Adds a command which records info about the next cursorless command and saves it to a yaml fixture:
On recording the user is prompted for the talon command and test fixture file nam
During testing the editor initial state is restored, the recorded command is run, and the result is compared to the recorded one.