-
Notifications
You must be signed in to change notification settings - Fork 119
feat: make the integ test harness work on Windows #1854
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
base: main
Are you sure you want to change the base?
Changes from all commits
7d44eeb
6c5b411
34a9efd
dcb3b67
8be4c48
d936cad
628d0e3
56685e6
dcf9d18
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,6 @@ | ||
| /** | ||
| * Whether the current process is running on Windows. | ||
| */ | ||
| export function isWindows(): boolean { | ||
| return process.platform === 'win32'; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import * as fs from 'fs'; | |
| import * as os from 'os'; | ||
| import * as path from 'path'; | ||
| import type { TestContext } from './integ-test'; | ||
| import { isWindows } from './platform'; | ||
| import { Process } from './process'; | ||
| import type { TemporaryDirectoryContext } from './with-temporary-directory'; | ||
|
|
||
|
|
@@ -282,7 +283,25 @@ export class ShellHelper { | |
| export function rimraf(fsPath: string): boolean { | ||
| try { | ||
| let success = true; | ||
| const isDir = fs.lstatSync(fsPath).isDirectory(); | ||
| const stat = fs.lstatSync(fsPath); | ||
|
|
||
| // This test's private directory contains a 'node_modules' symlink into a | ||
| // machine-wide shared install that other running tests also link to. Delete | ||
| // the link itself and stop — do NOT recurse through it, or we'd delete the | ||
| // shared install's contents out from under those other tests. | ||
| if (stat.isSymbolicLink()) { | ||
| // On POSIX, unlink removes a symlink whatever its target type. On | ||
| // Windows, a link to a directory (or a junction) must be removed with | ||
| // rmdir, while a link to a file must be removed with unlink. | ||
| if (isWindows() && isDirectoryLink(fsPath)) { | ||
| fs.rmdirSync(fsPath); | ||
|
iliapolo marked this conversation as resolved.
|
||
| } else { | ||
| fs.unlinkSync(fsPath); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| const isDir = stat.isDirectory(); | ||
|
|
||
| if (isDir) { | ||
| for (const file of fs.readdirSync(fsPath)) { | ||
|
|
@@ -309,14 +328,26 @@ export function rimraf(fsPath: string): boolean { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Whether a symlink resolves to a directory. | ||
| * | ||
| * `statSync` follows the link, so a directory target means a directory link. | ||
| * A dangling link (target already removed) returns undefined; treat it as a | ||
| * directory, since the only links we create are directory links (the shared | ||
| * 'node_modules' junction) and those still need `rmdir` on Windows. | ||
| */ | ||
| function isDirectoryLink(linkPath: string): boolean { | ||
| return fs.statSync(linkPath, { throwIfNoEntry: false })?.isDirectory() ?? true; | ||
|
Contributor
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. This function is not needed. But in general - is there a good reason why we would call this on a path that doesn't exist? Agents will always prefer not to throw - make sure you evaluate that decision every time, because this hides very subtle bugs.
Contributor
Author
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. I didn't understand - when would the path not exist?
Contributor
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. Exactly - it should always exist, which is why we should throw |
||
| } | ||
|
|
||
| export function addToShellPath(x: string) { | ||
| const parts = process.env.PATH?.split(':') ?? []; | ||
| const parts = process.env.PATH?.split(path.delimiter) ?? []; | ||
|
|
||
| if (!parts.includes(x)) { | ||
| parts.unshift(x); | ||
| } | ||
|
|
||
| process.env.PATH = parts.join(':'); | ||
| process.env.PATH = parts.join(path.delimiter); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -339,7 +370,28 @@ export function addToShellPath(x: string) { | |
| class LastLine { | ||
| private lastLine: string = ''; | ||
|
|
||
| // win32 only: the last completed line that had visible content, see below | ||
| private lastVisibleLine: string = ''; | ||
|
|
||
| public append(chunk: string): void { | ||
| if (isWindows()) { | ||
| // ConPTY renders the screen buffer instead of streaming plain text: | ||
| // prompts are drawn with cursor-positioning escape sequences, padded | ||
| // with spaces to the terminal width, and followed by "lines" that | ||
| // contain nothing but more escape sequences. Match against the last | ||
| // line that had visible content, so control-only lines don't erase a | ||
| // prompt that was just drawn. | ||
| const lines = stripAnsi(chunk).split(/\r?\n/); | ||
| this.lastLine += lines[0]; | ||
| for (const line of lines.slice(1)) { | ||
| if (this.lastLine.trim().length > 0) { | ||
| this.lastVisibleLine = this.lastLine; | ||
| } | ||
| this.lastLine = line; | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const lines = chunk.split(os.EOL); | ||
| if (lines.length === 1) { | ||
| // chunk doesn't contain a new line so just append | ||
|
|
@@ -351,10 +403,30 @@ class LastLine { | |
| } | ||
|
|
||
| public get(): string { | ||
| if (isWindows() && this.lastLine.trim().length === 0) { | ||
| return this.lastVisibleLine; | ||
| } | ||
| return this.lastLine; | ||
| } | ||
|
|
||
| public reset() { | ||
| this.lastLine = ''; | ||
| this.lastVisibleLine = ''; | ||
| } | ||
| } | ||
|
|
||
| const ESC = '\u001b'; | ||
| // CSI sequences (cursor movement, erase, colors) and OSC sequences (window title) | ||
| const ANSI_REGEX = new RegExp(`${ESC}\\[[0-9;?]*[@-~]|${ESC}\\][^${ESC}\\u0007]*(?:\\u0007|${ESC}\\\\)`, 'g'); | ||
|
|
||
| /** | ||
| * Remove ANSI escape sequences from terminal output. | ||
| * | ||
| * Windows ConPTY renders the screen buffer rather than streaming plain text: | ||
| * once the cursor reaches the bottom of the buffer, lines arrive as absolute | ||
| * cursor-positioning sequences instead of newline-terminated text. Prompt | ||
| * matching must look at the text only. | ||
| */ | ||
| function stripAnsi(chunk: string): string { | ||
| return chunk.replace(ANSI_REGEX, ''); | ||
| } | ||
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 not:
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 used
lstatsyncearlier, which describes the symlink, and not the target. isDirectory needs to operate on the target.Ref - https://www.geeksforgeeks.org/node-js/node-js-fs-lstatsync-method/
https://www.geeksforgeeks.org/node-js/node-js-stats-isdirectory-method-from-fs-stats-class/
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.
Discussed offline, we are already not recursing if its a symlink to a directory, so we just need to handle the unliking correctly: