Skip to content

Conversation

@cap10morgan
Copy link
Contributor

@cap10morgan cap10morgan commented Nov 6, 2025

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. 😅

@cap10morgan cap10morgan force-pushed the chore/redo-migrated-unit-tests branch from eff5c8e to 2681807 Compare November 6, 2025 19:47
I'm planning to add this to a code style doc, probably in CONTRIBUTING.md
@cap10morgan cap10morgan force-pushed the chore/redo-migrated-unit-tests branch from 2681807 to 5b713b1 Compare November 6, 2025 19:55
Copy link
Member

@kriszyp kriszyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cap10morgan
Copy link
Contributor Author

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
Copy link
Member

kriszyp commented Nov 7, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants