Add Windows Support to Performance Wesley's Test Runner #45

GroophyLifefor · 2025-08-14T19:26:24Z

Summary

This PR adds Windows compatibility to the performance test runner script. Previously, the runner relied on Unix-specific shell commands, which prevented it from working on Windows. The following changes ensure the runner works cross-platform

…ows-support

wesleytodd

Overall I think my biggest issue is that this does a ton of non-windows support things. It makes it difficulty to follow what is for the stated goal and what is refactors (maybe some good and worthy ones, but hard to tell all mixed together). I think most of my comments are about that, but I probably have a few nitpicks as well once we can get a PR that does just the one thing.

wesleytodd · 2025-08-15T14:32:15Z

packages/cli/bin/expf.mjs

@@ -1,3 +1,5 @@
+#!/usr/bin/env node


This file is not intended to be called directly. I see how it is paired with removing the .sh file but that is not going to work for this. Take a look at how npm does this, I would prefer we take a similar approach: https://github.com/npm/cli/tree/latest/bin

Specifically because I want to be able to pass flags to node, and you cannot do that without a wrapper script.

I didn't want to break Unix while ensuring Windows compatibility. It's good that I learned this. I'll remove this line.

wesleytodd · 2025-08-15T14:33:24Z

packages/cli/bin/expf.mjs

+main().catch((error) => {
+  console.error('CLI error:', error.message);
+  process.exit(1);
+});


I am not clear on how these changes in resolve windows compatibility. Can you help me understand them better?

It's not directly Windows compatibility change, but to understand problems helped me to catch errors.

wesleytodd · 2025-08-15T14:34:28Z

packages/cli/package.json

  },
  "bin": {
-    "expf": "./bin/expf.sh"
+    "expf": "./bin/expf.mjs"


Just commenting here to connect the dots to the other comment. We need to keep entry scripts that allow specifying node options. We may not need something as complicated as what npm does, but we at least need to keep the shell script wrappers.

May I don't understand this perfectly. when we put sh file to bin, is that someway npm handles it?

All npm and the various package managers do is create links from node_modules/.bin/expf -> node_modules/@expressjs/perf-cli/bin/expf.sh. So it can be any type of executable.

wesleytodd · 2025-08-15T14:35:22Z

packages/runner-docker/.gitignore

@@ -0,0 +1,2 @@
+.dd_key
+.do_key


I removed these, must need to rebase this branch?

wesleytodd · 2025-08-15T14:36:58Z

packages/runner-docker/Dockerfile

-EOF
+RUN npm install -g @mmarchini/observe@^3.0.0 && \
+    chown -R node:node /usr/local/lib/node_modules && \
+    chown -R node:node /usr/local/bin/observe


IIRC I did this because there is an error mode that makes this form harder to debug. Is this required for windows compat for some reason?

From mine bash knownledge, it's for doing multi-line command execution in one time, so should not difference in runtime. but if causes an error ofc we can use EOF.

It is not that it causes an error. IIRC I broke them apart because when there is an error the message is harder to read and debug.

wesleytodd · 2025-08-15T14:39:58Z

packages/runner-docker/index.mjs

@@ -1,76 +1,51 @@
 import { execFile } from 'node:child_process';


This file reads as a set of refactors, not things that are needed for windows compat. Can you help me follow what the goal is with these changes?

Looks some of caused because of wrong conflict selection, like imports, in the other additions are mostly for logging. Need to fixed.

wesleytodd · 2025-08-15T14:42:53Z

packages/runner-docker/scripts/build.mjs

@@ -0,0 +1,129 @@
+#!/usr/bin/env node


Honestly it was files like this that I expected to see, would you be able to extract just the changes that make the local file stuff use JS instead of bash into a new PR? That would really help make this stuff easier to review and land.

I can try in a new branch with re-fix windows compatibility with current codes with just change most less code, like core sh files to js that not in docker.

wesleytodd · 2025-08-15T14:43:35Z

packages/runner-docker/scripts/build.mjs

+const execAsync = async (cmd, opts) => {
+  console.log(`Executing: ${cmd}`);
+  return promisify(exec)(cmd, opts);
+};


This function is unnecessary. Just wrap exec once at the top level.

I agree, that's is just for more logging to better understand problems and fix them.

wesleytodd · 2025-08-15T14:46:16Z

packages/runner-docker/scripts/build.mjs

+  } else {
+    // Linux - usually runs as a service
+    try {
+      await execAsync('sudo systemctl start docker');


We should not ever call sudo. I get what you are doing here, but if the script does not have the ability to start docker without sudo then we should not start it ourselves.

That's a good point that I don't thought before, we should not use sudo.

wesleytodd · 2025-08-15T14:48:48Z

packages/runner-docker/scripts/start.mjs

@@ -0,0 +1,366 @@
+#!/usr/bin/env node


This file does not need to be re-written. The container is always linux and thus everything going on in that container will be cross-system compatible.

If you want to make the case that bash is harder to deal with than JS, then lets discuss that in a thread dedicated to the topic, but I would like to see a PR that is JUST the changes that add windows compatibility.

Actually the core reason why I made replaced all sh files to js one is a full convertion, but when I look now, I saw this is unnecessary. Only required sh files changes enought as you said.

GroophyLifefor · 2025-08-15T16:36:54Z

Overall I think my biggest issue is that this does a ton of non-windows support things. It makes it difficulty to follow what is for the stated goal and what is refactors (maybe some good and worthy ones, but hard to tell all mixed together). I think most of my comments are about that, but I probably have a few nitpicks as well once we can get a PR that does just the one thing.

As I said in the meeting, I agree with you. I was inspired by your approach in the CI-based perf-runner, but I solved problems.

Firstly, I kept Dockerization within the Docker image, not any other, only Dockerimage code.
I created a way to use templates so that you don't have to write an autocannon setup for each test. It can also be customized for each repository.
I tried to split it into parts as you do with packages. I only kept the Docker image in this repository, and the tests are written in the repositories. This makes it easier to focus on the runner and the output of the process.

I briefly reviewed your comments. Some of looking caused when I tried to resolve some conflicts, some of which were my fault. However, when I have more flexible time, I would like to review each comment thoroughly. These are just my thoughts.

wesleytodd and others added 3 commits June 23, 2025 11:11

feat: a cli for performance testing

644cb15

Merge branch 'tools' of https://github.com/expressjs/perf-wg into tools

4d035f0

WindowsOS focused cross-platform support

583bfa2

github-project-automation bot added this to Perf WG Tooling Aug 14, 2025

Merge branch 'main' of https://github.com/expressjs/perf-wg into wind…

5be13d5

…ows-support

wesleytodd requested changes Aug 15, 2025

View reviewed changes

wesleytodd mentioned this pull request Aug 20, 2025

Proposal PR: CI-based performance testing with Docker runner #43

Open

		@@ -1,76 +1,51 @@
		import { execFile } from 'node:child_process';

		@@ -0,0 +1,2 @@
		.dd_key
		.do_key

Uh oh!

Add Windows Support to Performance Wesley's Test Runner #45

Are you sure you want to change the base?

Add Windows Support to Performance Wesley's Test Runner #45

Uh oh!

Conversation

GroophyLifefor commented Aug 14, 2025

Summary

Uh oh!

wesleytodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GroophyLifefor commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GroophyLifefor commented Aug 15, 2025 •

edited

Loading