feat: make the integ test harness work on Windows - #1854
Conversation
dgandhi62
changed the title
dgandhi62
changed the title
| // still. Install every distinct package set only once per machine and | ||
| // junction it into the test directory. | ||
| const sharedNodeModules = await sharedPackageSetInstall(fixture, packages); | ||
| fs.symlinkSync(sharedNodeModules, path.join(fixture.integTestDir, 'node_modules'), 'junction'); |
There was a problem hiding this comment.
Interesting. Lets make use of it on linux as well! We've seen network errors due to multiple npm install processes and I remember also seeing some slow installation in our CodeBuild canary runs.
There was a problem hiding this comment.
This was one of the biggest changes for bringing down Windows integ test times, since npm install is so much less efficient. Concur on running it on Linux. @dgandhi62 thanks for the changes, can you show us a workflow run using the latest commit to validate it works as expected (tests pass and shorter test times)?
There was a problem hiding this comment.
Running it with the latest code now. Will link it here once done
There was a problem hiding this comment.
| await context.library.initializeDotnetPackages(context.integTestDir); | ||
| await shell.shell(['cdk', 'synth']); | ||
| }))); | ||
| })), 300_000); |
There was a problem hiding this comment.
For each instance where we adjust test timeouts, please verify in the workflow run whether it needs this much time.
There was a problem hiding this comment.
I can reduce the timeouts in some to make it closer to the test times. Depending on the urgency of this pr, we can have a separate one to make efficiency improvements in the tests themselves
There was a problem hiding this comment.
For the ask of updating timeouts based on test times, that is done. If the ask is to identify test improvements to reduce times, that might have to be a separate pr
There was a problem hiding this comment.
No ask for the latter. Reducing the timeouts is OK.
4e33fc8 to
a8f331f
Compare
The integ test harness and several suites assume a POSIX environment, so they cannot run on a Windows runner. This makes them platform-portable without changing behaviour on Linux. - spawn TTY processes through the shell, and widen the ConPTY terminal so long prompts are not wrapped before they are matched - match prompts against ConPTY screen-buffer output - spawn npm through the node interpreter rather than relying on bin shims - share one npm install across tests, which dominates runtime on Windows - fix path handling in the watch tests and search all stage assemblies for the nested template - give the init suites an explicit 5 minute timeout; they previously ran on the 60s suite default, which is not enough for Maven, NuGet or Go module downloads No Windows jobs run yet; enabling those is a follow-up.
The shared install added for Windows applies just as well everywhere:
every test asks for the same handful of packages at the same resolved
versions, so installing per test is duplicated work. On Linux that shows
up as many concurrent `npm install` processes, which is a known source of
ECONNRESET failures, and as slow installs in the CodeBuild canary runs.
- drop the win32 gate, and pick the symlink type per platform ('junction'
on Windows, where a 'dir' symlink needs elevation; ignored on POSIX)
- keep per-test installs when REPO_ROOT rewrites a package to a local
directory: the cache is keyed on the requested package set, and a
directory path does not change when its contents are rebuilt, so
sharing there would serve stale code. No package installed here is
currently a workspace of this repo, so this is a guard, not a fix
- coordinate through the existing XpMutex instead of a hand-rolled lock
directory. It reclaims a lock whose owner has died by checking pid
liveness, rather than waiting out a timeout: previously a worker killed
mid-install left a lock nothing would release, so every other test on
the machine waited out the 30 minute deadline and failed
Keying on the package set is safe because `requestedVersion()` always
resolves to an exact version before it reaches the installer.
Addresses review feedback on the shared-install block.
The cross-process mutex guarding the shared npm install represents a lock as a file: acquire by exclusively creating it, release by unlinking it. This assumes POSIX deletion semantics, where the only "can't create" signal is EEXIST and the only "can't read" signal is ENOENT. Windows differs. A lock file that another process still has open, or that was just unlinked, enters a "delete pending" state: it lingers in the directory but open()/read() against it fail with EPERM/EACCES. Under the heavy startup contention the shared install creates (every jest worker races for the same lock), tryAcquire() hit EPERM, fell into the `code !== 'EEXIST'` branch, and rethrew a fatal error. On the Windows integ runner this took down every test in the suite with an identical 'EPERM: operation not permitted, open ...cdk-integ-shared-install...mutex'. Treat EPERM/EACCES the same as the POSIX signals: on exclusive create they mean "held or mid-transition, back off and retry" (like EEXIST); on read they mean "not readable, treat as gone" (like ENOENT). Add a short sleep before retrying so a persistent delete-pending window does not busy-spin. POSIX behavior is unchanged: EPERM does not occur on this path there, so the new branches are inert on Linux and macOS. Also bump the init-typescript-app integ test timeouts (300s->600s, 180s->300s) to account for the slower Windows runners.
…changes. No behavioral change on any platform. - add an isWindows() util and route the harness's explicit win32 checks through it, so platform branches read plainly instead of comparing process.platform inline - in rimraf, replace the try/catch that used a failed unlink to detect a Windows junction with an explicit branch: unlink on POSIX, and on Windows pick rmdir vs unlink based on whether the link actually resolves to a directory (isDirectoryLink) rather than assuming every link is a directory - reword the rimraf symlink comment to make the link-vs-target boundary explicit: we delete this test's private node_modules link and stop, never recursing into the shared install other running tests depend on - inline the single-use RENAME_RETRYABLE array in atomicWrite
| // 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)) { |
There was a problem hiding this comment.
Why not:
| if (isWindows() && isDirectoryLink(fsPath)) { | |
| if (isWindows() && stats.isDirectory()) { |
There was a problem hiding this comment.
We used lstatsync earlier, 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.
Discussed offline, we are already not recursing if its a symlink to a directory, so we just need to handle the unliking correctly:
// 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 (stat.isSymbolicLink() && isWindows() && isDirectoryLink(fsPath)) {
fs.rmdirSync(fsPath);
} else {
fs.unlinkSync(fsPath);
}| * 'node_modules' junction) and those still need `rmdir` on Windows. | ||
| */ | ||
| function isDirectoryLink(linkPath: string): boolean { | ||
| return fs.statSync(linkPath, { throwIfNoEntry: false })?.isDirectory() ?? true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't understand - when would the path not exist?
There was a problem hiding this comment.
Exactly - it should always exist, which is why we should throw
| * On Windows the CLI is an npm .cmd shim, which `spawn` can only start | ||
| * through a shell ('spawn cdk ENOENT' otherwise). | ||
| */ | ||
| export function spawnWatch(args: string[], options: SpawnOptions): ChildProcess { |
There was a problem hiding this comment.
Why is this called spawnWatch but actually just runs cdk? Also I'm sure we already have this function somewhere that can be reused.
There was a problem hiding this comment.
I'm not sure why (I inherited this code from the original pr). The function is here -
My guess is that we need a way to kill the process at the end specifically for watch, and returning the child process gives us that. The return type of the older function is completely non-destructive.
I think this might explain the naming too
There was a problem hiding this comment.
If you look at the integ tests, we've just abstracted away common code. I don't think this is a big change per say
There was a problem hiding this comment.
I was just confused by the name because it doesn't actually spawn cdk watch. Its fine.
4 participants
This pr is created for maintaining separation of concerns from here - #1781
Here, we make modifications to the testing harness, since a lot of the older code was linux-specific and did not run properly on Windows. The code to enable the windows tests will be a follow up.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license