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.

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

Merged
merged 40 commits into from
Jul 31, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
f7587e3
Very preliminary steps
pokey Jul 3, 2021
815e829
Write test case info to file
brxck Jul 7, 2021
ed4ee0d
Save only target decorations
brxck Jul 7, 2021
3d0e8b0
Don't use yaml refs in test case fixtures
brxck Jul 8, 2021
e9b60e4
Record clipboard contents in snapshots
brxck Jul 9, 2021
efb696c
Run recorded test cases
brxck Jul 10, 2021
e95a993
Only save clipboard contents in copy/paste actions
brxck Jul 13, 2021
b06f27f
Reuse navigation map key creation
brxck Jul 13, 2021
c51def0
Add inferred full targets to fixture
brxck Jul 13, 2021
cc0e4bf
Move test fixtures from src to root
brxck Jul 14, 2021
9d65fde
Remove setting up navigation map
brxck Jul 14, 2021
fa52794
Attempt to mock clipboard (and fail)
brxck Jul 14, 2021
35a8ce5
Add navigationMap to graph
pokey Jul 13, 2021
0471c7f
Attempt to mock navigation map construction
brxck Jul 14, 2021
09a743f
Store "that mark" in test case fixture
brxck Jul 15, 2021
43a4473
Check nav map and improve that mark handling
brxck Jul 17, 2021
829882d
Give up for the night lol
brxck Jul 17, 2021
087c7fd
Support testing that mark
brxck Jul 18, 2021
50ff1e4
Add mockable clipboard layer
brxck Jul 18, 2021
f6db9e4
Test command return value
brxck Jul 18, 2021
be29b40
Only save targeted that marks to fixture
brxck Jul 18, 2021
84326b4
Name fixture and include Talon command
brxck Jul 18, 2021
c6cd5ec
Only save visible ranges for folding actions
brxck Jul 18, 2021
ca334b2
Start refactoring test cases
brxck Jul 27, 2021
01b8553
Fix that mark detection on compound targets
brxck Jul 27, 2021
c5c3d9b
Improve navigation map handling
brxck Jul 28, 2021
060d91d
Clarify thatMark exclusion conditions
brxck Jul 28, 2021
08ebc6b
Rename fixture fields
brxck Jul 28, 2021
076da28
Rephrase 'serialize' as 'to plain object'
brxck Jul 28, 2021
52f4408
Add issue for visible range testing
brxck Jul 28, 2021
ff6a1e1
Revert makeGraph export change
brxck Jul 28, 2021
f585de0
Improve fixture writing and move directory
brxck Jul 30, 2021
b8df923
Add test case folder selection
brxck Jul 30, 2021
decc22f
Create test case recorder class
brxck Jul 30, 2021
16f67bb
Ask for fixture directory after recording
brxck Jul 30, 2021
0b8df41
Move file handling to test case recorder
brxck Jul 30, 2021
3f188a3
Cleanup
pokey Jul 31, 2021
a255e0b
More cleanup
pokey Jul 31, 2021
8ce8f86
cleanup
pokey Jul 31, 2021
39d62f5
swap anchor with active
pokey Jul 31, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
{
"command": "cursorless.recomputeDecorationStyles",
"title": "Cursorless: Recompute decoration styles"
},
{
"command": "cursorless.recordTestCase",
"title": "Cursorless: Record test case"
}
],
"colors": [
Expand Down Expand Up @@ -196,15 +200,19 @@
},
"devDependencies": {
"@types/glob": "^7.1.3",
"@types/js-yaml": "^4.0.2",
"@types/mocha": "^8.0.4",
"@types/node": "^12.11.7",
"@types/sinon": "^10.0.2",
"@types/vscode": "^1.53.0",
"@typescript-eslint/eslint-plugin": "^4.9.0",
"@typescript-eslint/parser": "^4.9.0",
"esbuild": "^0.11.12",
"eslint": "^7.15.0",
"glob": "^7.1.6",
"js-yaml": "^3.14.1",
"mocha": "^8.1.3",
"sinon": "^11.1.1",
"typescript": "^4.1.2",
"vscode-test": "^1.4.1"
},
Expand All @@ -213,4 +221,4 @@
"immutability-helper": "^3.1.1",
"lodash": "^4.17.21"
}
}
}
12 changes: 12 additions & 0 deletions src/Clipboard.ts
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;
}
19 changes: 16 additions & 3 deletions src/NavigationMap.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { TextDocumentChangeEvent } from "vscode";
import { SymbolColor } from "./constants";
import { SerializedRange, serializeRange } from "./TestCase";
import { Token } from "./Types";

/**
Expand Down Expand Up @@ -43,15 +44,27 @@ export default class NavigationMap {
[coloredSymbol: string]: Token;
} = {};

private getKey(color: SymbolColor, character: string) {
static getKey(color: SymbolColor, character: string) {
return `${color}.${character}`;
}

public addToken(color: SymbolColor, character: string, token: Token) {
this.map[this.getKey(color, character)] = token;
this.map[NavigationMap.getKey(color, character)] = token;
}

public getToken(color: SymbolColor, character: string) {
return this.map[this.getKey(color, character)];
return this.map[NavigationMap.getKey(color, character)];
}

public serializeRanges() {
const rangeMap: { [coloredSymbol: string]: SerializedRange } = {};
Object.entries(this.map).forEach(([key, value]) => {
rangeMap[key] = serializeRange(value.range);
});
return rangeMap;
}
Copy link
Member

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?


public clear() {
this.map = {};
}
}
241 changes: 241 additions & 0 deletions src/TestCase.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
import * as path from "path";
import * as fs from "fs";
import * as yaml from "js-yaml";
import * as vscode from "vscode";
import { Position, Range, Selection } from "vscode";
import { Clipboard } from "./Clipboard";
import NavigationMap from "./NavigationMap";
import { ThatMark } from "./ThatMark";
import {
ActionType,
PartialTarget,
PrimitiveTarget,
SelectionWithEditor,
Target,
} from "./Types";

export type SerializedPosition = {
line: number;
character: number;
};

export type SerializedRange = {
start: SerializedPosition;
end: SerializedPosition;
};

export type SerializedSelection = {
anchor: SerializedPosition;
active: SerializedPosition;
};

export function serializeRange(range: Range): SerializedRange {
return {
start: serializePosition(range.start),
end: serializePosition(range.end),
};
}

export function serializeSelection(selection: Selection): SerializedSelection {
return {
active: serializePosition(selection.active),
anchor: serializePosition(selection.anchor),
};
}

export function serializePosition(position: Position): SerializedPosition {
return { line: position.line, character: position.character };
}

type TestCaseCommand = {
actionName: ActionType;
partialTargets: PartialTarget[];
Copy link
Member

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

extraArgs: any[];
};

type TestCaseContext = {
talonCommand: string;
thatMark: ThatMark;
targets: Target[];
Copy link
Member

Choose a reason for hiding this comment

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

It's a funny smell to have targets here but the actual cursorless command somewhere else

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. { command, talonCommand, thatMark, targets } ?

navigationMap: NavigationMap;
};

type TestCaseSnapshot = {
document: string;
clipboard: string;
visibleRanges: SerializedRange[];
selections: SerializedSelection[];
thatMark: SerializedSelection[] | null;
};
Copy link
Member

Choose a reason for hiding this comment

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

I would make visibleRanges, clipboard, and thatMark optional, and then not include them rather than having them be empty / null


type DecorationRanges = { [coloredSymbol: string]: SerializedRange };

export type TestCaseFixture = {
talonCommand: string;
command: TestCaseCommand;
targets: Target[];
languageId: string;
marks: DecorationRanges;
initialState: TestCaseSnapshot;
finalState: TestCaseSnapshot;
returnValue: any;
};

export default class TestCase {
Copy link
Member

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?

talonCommand: string;
command: TestCaseCommand;
languageId: string;
targets: Target[];
marks: DecorationRanges;
context: TestCaseContext;
initialState: TestCaseSnapshot | null = null;
finalState: TestCaseSnapshot | null = null;
returnValue: any = null;

constructor(command: TestCaseCommand, context: TestCaseContext) {
const activeEditor = vscode.window.activeTextEditor!;
const { navigationMap, targets, talonCommand } = context;

this.talonCommand = talonCommand;
this.command = command;
this.languageId = activeEditor.document.languageId;
this.marks = this.extractTargetedDecorations(targets, navigationMap);
this.targets = targets;
this.context = context;
}

extractPrimitiveTargetKeys(...targets: PrimitiveTarget[]) {
Copy link
Member

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

const keys: string[] = [];
targets.forEach((target) => {
if (target.mark.type === "decoratedSymbol") {
const { character, symbolColor } = target.mark;
keys.push(NavigationMap.getKey(symbolColor, character));
}
});
return keys;
}

extractTargetKeys(target: Target): string[] {
switch (target.type) {
case "primitive":
return this.extractPrimitiveTargetKeys(target);

case "list":
return target.elements.map(this.extractTargetKeys, this).flat();

case "range":
return this.extractPrimitiveTargetKeys(target.start, target.end);

default:
return [];
}
}

extractTargetedDecorations(targets: Target[], navigationMap: NavigationMap) {
if (!navigationMap) {
return {};
}

const decorationRanges = navigationMap.serializeRanges();
Copy link
Member

Choose a reason for hiding this comment

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

Seems wasteful / not quite the right separation of concerns to have navigation map dump everything. Might be better to do getToken directly for what you need

const targetedDecorations: DecorationRanges = {};
const targetKeys = targets.map(this.extractTargetKeys, this).flat();
targetKeys.forEach((key) => {
targetedDecorations[key] = decorationRanges[key];
});
return targetedDecorations;
}

isThatMarkTargeted() {
Copy link
Member

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

return this.targets.some(
(target) => target.type === "primitive" && target.mark.type === "that"
);
}

static async getSnapshot(
thatMark: SelectionWithEditor[]
): Promise<TestCaseSnapshot> {
const activeEditor = vscode.window.activeTextEditor!;
return {
document: activeEditor.document.getText(),
selections: activeEditor.selections.map(serializeSelection),
visibleRanges: activeEditor.visibleRanges.map(serializeRange),
clipboard: await Clipboard.readText(),
thatMark: thatMark.map((mark) => serializeSelection(mark.selection)),
};
}

async getSnapshot(): Promise<TestCaseSnapshot> {
Copy link
Member

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

return await TestCase.getSnapshot(this.context.thatMark.get());
}

async saveSnapshot() {
Copy link
Member

Choose a reason for hiding this comment

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

The difference between saveSnapshot, the two different getSnapshot functions, and writeSnapshot is a bit confusing. Might rework / rename

const snapshot = await this.getSnapshot();

if (!["copy", "paste"].includes(this.command.actionName)) {
snapshot.clipboard = "";
Copy link
Member

Choose a reason for hiding this comment

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

i'm not in love with setting things then deleting them later, tho not a showstopper

}

if (!["fold", "unfold"].includes(this.command.actionName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!["fold", "unfold"].includes(this.command.actionName)) {
if (
![
"fold",
"unfold",
"scrollToBottom",
"scrollToCenter",
"scrollToTop",
].includes(this.command.actionName)
) {

snapshot.visibleRanges = [];
}

if (this.initialState == null && !this.isThatMarkTargeted()) {
snapshot.thatMark = [];
}

if (this.initialState == null) {
this.initialState = snapshot;
} else if (this.finalState == null) {
this.finalState = snapshot;
} else {
throw Error("Both snapshots already taken");
}

return snapshot;
}

toYaml() {
if (this.initialState == null || this.finalState == null) {
throw Error("Two snapshots must be taken before serializing");
}
const fixture: TestCaseFixture = {
talonCommand: this.talonCommand,
command: this.command,
languageId: this.languageId,
targets: this.targets,
marks: this.marks,
initialState: this.initialState,
finalState: this.finalState,
returnValue: this.returnValue,
};
return yaml.dump(fixture, { noRefs: true, quotingType: '"' });
}

async writeFixture(filename: string) {
if (filename === "") {
throw new Error("Filename required");
}

const fixture = this.toYaml();
const workspacePath = vscode.workspace.workspaceFolders?.[0].uri.path;
let document;

if (workspacePath) {
Copy link
Member

Choose a reason for hiding this comment

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

when will this one be null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are in a fresh, untitled window, ex. File > New Window, or if only a file is opened, not a folder.

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,
});
}
}
13 changes: 13 additions & 0 deletions src/ThatMark.ts
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;
}
}
1 change: 1 addition & 0 deletions src/Types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ export type ActionRecord = Record<ActionType, Action>;
export interface Graph {
readonly actions: ActionRecord;
readonly editStyles: EditStyles;
readonly navigationMap: NavigationMap;
}

export interface DecorationColorSetting {
Expand Down
11 changes: 6 additions & 5 deletions src/addDecorationsToEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ interface CharacterTokenInfo {
tokenIdx: number;
}

export function addDecorationsToEditors(decorations: Decorations) {
export function addDecorationsToEditors(
navigationMap: NavigationMap,
decorations: Decorations
) {
navigationMap.clear();

var editors: vscode.TextEditor[];

if (vscode.window.activeTextEditor == null) {
Expand Down Expand Up @@ -85,8 +90,6 @@ export function addDecorationsToEditors(decorations: Decorations) {
])
);

const navigationMap = new NavigationMap();

// Picks the character with minimum color such that the next token that contains
// that character is as far away as possible.
// TODO: Could be improved by ignoring subsequent tokens that also contain
Expand Down Expand Up @@ -151,6 +154,4 @@ export function addDecorationsToEditors(decorations: Decorations) {
editor.setDecorations(decorations.decorationMap[color]!, ranges[color]!);
});
});

return navigationMap;
}