Redo component unit test migration #31

cap10morgan · 2025-11-06T18:57:41Z

Alright, here's version 2.0 of how we might bring unit tests over from the harperdb repo.

Things to note:

This time we're keeping the tests as JS (not TS) but we may revisit this in the future
One of our goals is to run these tests against the compiled TS in the dist dir. So be sure to run npm run build before you run these tests (which you do via npm run test:unit:all)
I'm still very motivated to avoid relative requires in the tests (especially into the ./dist dir), so this uses Node's subpath import patterns feature to accomplish that. Once we're in TypeStrip land and running unit tests against that, we can just change the target of that pattern in package.json.
The component tests already followed the *.test.* filename pattern so no further renaming was needed.
If you want to know how to configure WebStorm to run these tests from the gutter "green arrow" icons, ping me in Slack. I've got a good setup, but it wasn't immediately obvious what all I needed to do.

Let's take our time reviewing this so we don't have to redo it again. 😅

kriszyp

This is fascinating approach, very interesting to learn this is possible. A couple questions:

Does this not work with ESM? It looks you were converting to CJS; is that for consistency or because imports don't respect subpath import definitions?
Does this work with Bun?

I was thinking that using TypeStrip was actually less constrained by any global switch of the code base (most of our source code is TypeStrip compatible), and more constrained by the tests themselves (that's where the biggest incompatilities with TS ESM exist, IIUC). That is, some existing tests still rely on mutating exports and some don't. Wouldn't it make sense for individual tests to reference './dist/' if they need to mutate exports ("test doubles"), and tests that don't need to do that can directly reference source? This would allow the tests to be transitioned over time to using source, with clear indication of which tests used source vs dist. And would remove the biggest blocker for us to be able to use TypeStrip SRTL.

kriszyp · 2025-11-07T13:20:09Z

unitTests/components/Application.test.ts

@@ -1,317 +0,0 @@
-/* eslint-disable @typescript-eslint/no-require-imports */


Why are we deleting this file (or the contents of it)? Is this a rename that I am missing?

I believe @Ethan-Arrowood is planning to bring it back after this lands. We discussed it and he said it would be fine to delete it for now to clear the slate for the redo.

Yeah for the sake of doing things right we can delete this new test file I created recently and add it back later following conventions.

cap10morgan · 2025-11-07T17:09:38Z

This is fascinating approach, very interesting to learn this is possible. A couple questions:

Does this not work with ESM? It looks you were converting to CJS; is that for consistency or because imports don't respect subpath import definitions?

I didn't so much convert to CJS as I just left them how them were in the harperdb repo (which was CJS).

The subpath import patterns should work fine with imports. In fact the examples in the docs are all imports.

Does this work with Bun?

Good question! I will investigate.

I was thinking that using TypeStrip was actually less constrained by any global switch of the code base (most of our source code is TypeStrip compatible), and more constrained by the tests themselves. That is, some existing tests still rely on mutating exports and some don't. Wouldn't it make sense for individual tests to reference './dist/' if they need to mutate exports ("test doubles"), and tests that don't need to do that can directly reference source? This would allow the tests to be transitioned over time to using source, with clear indication of which tests used source vs dist. And would remove the biggest blocker for us to be able to use TypeStrip SRTL.

We could certainly explore that approach, but I thought we had decided to run all unit tests against the CJS artifact we ship to users in ./dist. So that was the goal I was working towards.

kriszyp · 2025-11-07T20:30:35Z

I didn't so much convert to CJS as I just left them how them were in the harperdb repo (which was CJS).

Ah, I see, great.

but I thought we had decided to run all unit tests against the CJS artifact we ship to users in ./dist

Yeah, we have. And starting with tests using dist sounds good. But it is certainly not the ultimate goal, right? We do want to be able to run against source eventually. And so I think the crux question is what that transition looks like. If it is matter of making some fixes to the source code and then "flipping the switch" for the tests, then the #harper subpath sounds perfect. But, I think the reality is that the source code is probably pretty close to ready, and it is more of a transition of updating tests to be TypeStrip (ESM really) compatible (since so many try to mutate exports). And so this is more of a switch at the individual test level. And if that is the case, it seems like switching by changing paths from './dist', to '.' when a test is deemed ESM compatible, is the ideal transition step, isn't it?

cap10morgan · 2025-11-07T22:38:50Z

I didn't so much convert to CJS as I just left them how them were in the harperdb repo (which was CJS).

Ah, I see, great.

but I thought we had decided to run all unit tests against the CJS artifact we ship to users in ./dist

Yeah, we have. And starting with tests using dist sounds good. But it is certainly not the ultimate goal, right? We do want to be able to run against source eventually. And so I think the crux question is what that transition looks like. If it is matter of making some fixes to the source code and then "flipping the switch" for the tests, then the #harper subpath sounds perfect. But, I think the reality is that the source code is probably pretty close to ready, and it is more of a transition of updating tests to be TypeStrip (ESM really) compatible (since so many try to mutate exports). And so this is more of a switch at the individual test level. And if that is the case, it seems like switching by changing paths from './dist', to '.' when a test is deemed ESM compatible, is the ideal transition step, isn't it?

Yes, agreed. So we could do this with subpath import patterns too, if we wanted to (I do!). Something like #dist and #src? Seems nicer than relative paths to me, but I'm happy to use whatever the team & community prefer. I haven't had a chance to test it w/ Bun yet but am still planning to soon.

kriszyp · 2025-11-07T23:08:50Z

Something like #dist and #src?

🔥
Yeah, maybe check compatibility, but this would be pretty slick.

cap10morgan · 2025-11-10T16:30:48Z

Something like #dist and #src?

🔥 Yeah, maybe check compatibility, but this would be pretty slick.

I made that change and tested it with bun. It all seemed to work!

I also converted components/componentLoader.ts to use #src import paths. I'm not sure if we want to do that conversion now / in this PR. Mostly it demonstrates that it works (with the array replacement patterns, etc.). Happy to undo it, apply it further, or leave it as-is (in this branch) for now.

cap10morgan · 2025-11-10T16:32:03Z

...although I wasn't seeing these CI errors locally. Will investigate.

cap10morgan · 2025-11-25T19:25:07Z

Strange. bun run test:unit:typestrip:all works just fine locally for me. It fails here with an error that makes it seem like it doesn't understand TypeScript source. Will keep investigating.

...so we don't have to use relative paths into dist/ in all of our unit tests

...to make it easier to use this in other tooling

I'm planning to add this to a code style doc, probably in CONTRIBUTING.md

...in preparation for adding a #src one too for things that want to require / import TypeStrip source code

...to test that the #src import alias pattern works

...which appears to help with the "#src" import alias pattern

...with import / require split on how they're handled. import is always TypeStrip, require is CJS by default, but TypeStrip when the "typestrip" Node condition is set. Component unit tests are all updated to use this, and some source code is to (just to make sure it's working).

...by moving the createReuseportFd stuff out to a CJS module that only exports the node-unix-socket stuff when not on a win32 platform

...to ensure we're pulling in compiled CJS when appropriate. Node 20 was having a hard time with some of these otherwise.

cap10morgan · 2025-12-04T23:08:46Z

Strange. bun run test:unit:typestrip:all works just fine locally for me. It fails here with an error that makes it seem like it doesn't understand TypeScript source. Will keep investigating.

Still haven't figured out what's going on here. Works every time locally and with the same version of bun as in CI.

heskew · 2025-12-05T02:14:05Z

Strange. bun run test:unit:typestrip:all works just fine locally for me. It fails here with an error that makes it seem like it doesn't understand TypeScript source. Will keep investigating.

Still haven't figured out what's going on here. Works every time locally and with the same version of bun as in CI.

Could be affected by the node version available during the run? I get various results depending on the node version I have available at the time of running bun run test:unit:typestrip:all.

heskew · 2025-12-05T04:31:36Z

Bun might need more specific scripts. Something like this?

diff --git a/.github/workflows/unit-test.yml b/.github/workflows/unit-test.yml
index 5d6446c..c752582 100644
--- a/.github/workflows/unit-test.yml
+++ b/.github/workflows/unit-test.yml
@@ -67,8 +67,5 @@ jobs:
       - name: Build
         run: bun run build || true # we currently have type errors so just ignore that
 
-      - name: Run tests (CJS)
-        run: bun run test:unit:all
-
-      - name: Run tests (TypeStrip)
-        run: bun run test:unit:typestrip:all
+      - name: Run tests (Bun)
+        run: bun run test:unit:bun:all
diff --git a/package.json b/package.json
index b6cae75..1b44065 100644
--- a/package.json
+++ b/package.json
@@ -40,6 +40,8 @@
 		"test:integration": "node integrationTests/utils/scripts/run.ts",
 		"test:unit": "npx mocha --config unitTests/.mocharc.json",
 		"test:unit:all": "npm run test:unit unitTests",
+		"test:unit:bun": "env NODE_OPTIONS='--conditions=bun' npx mocha --config unitTests/.mocharc.json --grep '#skip-typestrip' --invert unitTests",
+		"test:unit:bun:all": "npm run test:unit:bun",
 		"test:unit:typestrip": "env NODE_OPTIONS='--conditions=typestrip' npx mocha --config unitTests/.mocharc.json --grep '#skip-typestrip' --invert",
 		"test:unit:typestrip:all": "npm run test:unit:typestrip unitTests"
 	},
@@ -51,6 +53,7 @@
 		"#src/*": {
 			"import": "./*.ts",
 			"require": {
+				"bun": "./*.ts",
 				"typestrip": "./*.ts",
 				"default": "./dist/*.js"
 			}
@@ -58,6 +61,7 @@
 		"#js/*": {
 			"import": "./*.js",
 			"require": {
+				"bun": "./*.js",
 				"typestrip": "./*.js",
 				"default": "./dist/*.js"
 			}

cb1kenobi · 2025-12-05T04:39:46Z

You may be running into the same issue I did... you just need more bun:

bun --bun run test:unit:typestrip:all

heskew · 2025-12-05T14:48:09Z

You may be running into the same issue I did... you just need more bun:
bun --bun run test:unit:typestrip:all

Got a laugh reading out 'bun bun run' 😂

Still need to update the package.json imports and I get a segfault trying that :-/

panic(main thread): Segmentation fault at address 0x0
Crashed while loading native module: /Users/nathan/src/harper/harper/node_modules/@datadog/pprof/prebuilds/darwin-arm64/node-137.node
oh no: Bun has crashed. This indicates a bug in Bun, not your code.

https://bun.report/1.3.3/Mn1274e01cwjCqgggB___utuqnB______________2x8tjB_A2AA

kriszyp · 2025-12-05T16:38:18Z

Crashed while loading native module: /Users/nathan/src/harper/harper/node_modules/@datadog/pprof/prebuilds/darwin-arm64/node-137.node

@datadog/pprof is literally a V8 profiler (directly calls the V8 profiling APIs), so it doesn't make sense to run this on Bun. (I don't think Bun has any programmatic/runtime APIs for profiling, but does for heap snapshots).

Ethan-Arrowood · 2025-12-05T21:08:12Z

I do request that we don't overload this PR. If we can get unit tests running with Node.js lets just send it. That at least enables us to iterate on the core codebase more reliably. Adding Bun support is an awesome objective, but I hope not a requirement to get unit tests supported again here

kriszyp · 2025-12-05T21:24:20Z

Yeah, I know I had asked about Bun earlier in this ticket, but I was specifically asking to verify that subpath imports would work with Bun, and we weren't heading in a direction that we would have to revert. But, I didn't intend to require this PR to add support for Bun. I guess if we want to add scripts, so it is easy to start seeing what failures exist for Bun, that's fine, but definitely shouldn't be a blocker (or block CI passing or anything).

heskew · 2025-12-05T21:27:08Z

Just pull it from the workflow?

cap10morgan · 2025-12-08T16:30:30Z

Thanks for all of the suggestions and feedback, everyone! Bun absolutely does not need to hold this up. I mostly just wanted to make sure the environment discrepancy wasn't indicative of some deeper issue. It seems like it is not, and I have a good path forward there.

Next I need to figure out what's up with the integration tests.

It works and all tests pass locally, but I can't get it to work in CI and have already spent more time on it than is warranted for a nice-to-have for this particular PR

cap10morgan force-pushed the chore/redo-migrated-unit-tests branch 2 times, most recently from 2681807 to 5b713b1 Compare November 6, 2025 19:55

kriszyp reviewed Nov 7, 2025

View reviewed changes

cap10morgan force-pushed the chore/redo-migrated-unit-tests branch 2 times, most recently from 9929730 to 49d7a75 Compare November 14, 2025 19:25

cap10morgan mentioned this pull request Nov 25, 2025

Followup to PR #15 #20

Closed

cap10morgan force-pushed the chore/redo-migrated-unit-tests branch 2 times, most recently from cacdc60 to a870812 Compare November 25, 2025 19:07

cap10morgan added 15 commits December 4, 2025 14:46

Delete previous attempt at migrating component unit tests

f967ebb

Migrate component unit tests from harperdb repo

880a2f1

Add #harper import subpath pattern

ed16da2

...so we don't have to use relative paths into dist/ in all of our unit tests

Fix a require & an import so unit tests will pass

bb2a395

Ignore TS build failure in build script

9704ec5

...to make it easier to use this in other tooling

Standardize on arrow functions in unit tests

02f89aa

I'm planning to add this to a code style doc, probably in CONTRIBUTING.md

Change #harper import alias pattern to #dist

479e432

...in preparation for adding a #src one too for things that want to require / import TypeStrip source code

Add #src import alias pattern alias

f47b8ba

Use #src for componentLoader imports

575f3fb

...to test that the #src import alias pattern works

Commit prettier formatting changes

00549cd

Add "rootDir": "." to tsconfig.build.json

29d03d3

...which appears to help with the "#src" import alias pattern

Convert a couple modules to TS

391f5e3

Make serverHelpers/Request.ts TypeStrip compatible

07ece05

...by moving the createReuseportFd stuff out to a CJS module that only exports the node-unix-socket stuff when not on a win32 platform

Fix up some require / import paths

2edd0f8

...to ensure we're pulling in compiled CJS when appropriate. Node 20 was having a hard time with some of these otherwise.

cap10morgan added 5 commits December 4, 2025 14:47

Run TypeStrip unit tests on Node 22+ in CI

95ea5d2

Use latest checkout action in unit-test workflow

552c6d9

Run unit tests with Bun too

dabb629

Add bun.lock to .gitignore

b5efcb5

Update some more import paths

d585ad7

cap10morgan force-pushed the chore/redo-migrated-unit-tests branch from cfdf599 to d585ad7 Compare December 4, 2025 22:51

cap10morgan added 8 commits December 8, 2025 11:18

Add more bun to bun unit test workflows

e2c2255

Split bun unit tests into their own workflow

1c73e25

Try adding more bun again

bbf08d2

More bun for some, less bun for others?

68f7127

Remove bun unit test workflow for now

9f8e11a

It works and all tests pass locally, but I can't get it to work in CI and have already spent more time on it than is warranted for a nice-to-have for this particular PR

See if building Harper impacts integration tests

9cdfe0b

Add some test failure debugging msgs

6bee863

Import error msg objects in security/user.ts

8b6a3d5

		@@ -1,317 +0,0 @@
		/* eslint-disable @typescript-eslint/no-require-imports */

Redo component unit test migration #31

Are you sure you want to change the base?

Redo component unit test migration #31

Uh oh!

Conversation

cap10morgan commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kriszyp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kriszyp Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

cap10morgan Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Ethan-Arrowood Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

cap10morgan commented Nov 7, 2025

Uh oh!

kriszyp commented Nov 7, 2025

Uh oh!

cap10morgan commented Nov 7, 2025

Uh oh!

kriszyp commented Nov 7, 2025

Uh oh!

cap10morgan commented Nov 10, 2025

Uh oh!

cap10morgan commented Nov 10, 2025

Uh oh!

cap10morgan commented Nov 25, 2025

Uh oh!

cap10morgan commented Dec 4, 2025

Uh oh!

heskew commented Dec 5, 2025

Uh oh!

heskew commented Dec 5, 2025

Uh oh!

cb1kenobi commented Dec 5, 2025

Uh oh!

heskew commented Dec 5, 2025

Uh oh!

kriszyp commented Dec 5, 2025

Uh oh!

Ethan-Arrowood commented Dec 5, 2025

Uh oh!

kriszyp commented Dec 5, 2025

Uh oh!

heskew commented Dec 5, 2025

Uh oh!

cap10morgan commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cap10morgan commented Nov 6, 2025 •

edited

Loading

kriszyp left a comment •

edited

Loading