add Windows specifics to test subset file

[saidelike]

Checklist

• [/] I have added tests
• [/] I have updated the docs and cheatsheet
• [/] I have not broken the cheatsheet

[r-tae]

Sorry for the nitpick but technically its "MacOS" now

[saidelike]

Sorry for the nitpick but technically its "MacOS" now

Thanks for feedback. Fixed

[josharian]

I'd be tempted to normalize when reading this file instead, here:

cursorless/packages/test-harness/src/testSubset.ts

Lines 9 to 17 in 1df6f02

	export function testSubsetGrepString(): string {
	const inFile = testSubsetFilePath();
	return fs
	.readFileSync(inFile, "utf-8")
	.split(/\r?\n/)
	.map((line) => line.trim())
	.filter((line) => line.length > 0 && !line.startsWith("#"))
	.join("|");
	}

[pokey]

I'd actually argue to normalise when we construct the test name. See #1690 (comment). Looks like I forgot to file a follow-up issue for that 😅

[AndreasArvidsson]

I'm very much in favor of normalizing instead of having to do backslashes

[saidelike]

Done as advised. Tested on Windows only. Would be worth testing on Linux or OSX to make sure I didn't break anything and maybe on another Windows.

[pokey]

Looks like maybe you missed my comment above. I would argue for normalising here

cursorless/packages/common/src/testUtil/getFixturePaths.ts

Line 31 in 63ea8e9

name: path.relative(relativeDir, p.split(".")[0]),

[saidelike]

Looks like maybe you missed my comment above. I would argue for normalising here

cursorless/packages/common/src/testUtil/getFixturePaths.ts

Line 31 in 63ea8e9

name: path.relative(relativeDir, p.split(".")[0]),

Oh right I read your comment but missed it was a different place. I am not sure that place would be better as this is typically what we have on Windows:

name: recorded\actions\alternateHighlightHarp
path: C:\path\to\cursorless\packages\cursorless-vscode-e2e\src\suite\fixtures\recorded\actions\alternateHighlightHarp.yml

So you would want this changed to this right?

name: recorded/actions/alternateHighlightHarp
path: C:/path/to/cursorless/packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/actions/alternateHighlightHarp.yml

I am not sure if there is an efficient way in typescript to apply the "\" to "/" conversion for every single path/name. It seemed better (simpler) to me to do the conversion at the filter name and keep the Windows path names intact like I did in the commit?

[AndreasArvidsson]

I think I agree. If you ever print the path to the fixture and copy that you won't be able to use it on windows with forward slashes. It's the filter that should be separator agnostic not the file paths themselves that needs to be updated.

[pokey]

No I was proposing to normalize just the name, not the path. So it would be:

name: recorded/actions/alternateHighlightHarp
path: C:\path\to\cursorless\packages\cursorless-vscode-e2e\src\suite\fixtures\recorded\actions\alternateHighlightHarp.yml

The name is just a convenient string for displaying the test, and isn't really even a path, as it has the file extension stripped.

The problem with normalizing when reading the subset file is that it's just filtering test names, so the / there doesn't necessarily refer to a path. For example, if we had a test suite named check before / after, and you wanted to filter to just that test suite, the / would get transformed to \. The fact that some of our test names look like paths is just a convenient way to name them

That being said, I don't feel strongly, and I'm guessing that all test names that have / in them today do in fact refer to paths, and even if they don't, it's unlikely we'd use the / when filtering. So if the Windows folks wanna push for normalizing when reading subset file, I'll cave

[AndreasArvidsson]

At long it's just the name I'm totally fine with normalizing it.

[AndreasArvidsson]

Pokeys suggested fix is now merged: #2049
Closing this.