-
Notifications
You must be signed in to change notification settings - Fork 2
Redo component unit test migration #31
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?
Conversation
...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
eff5c8e to
2681807
Compare
I'm planning to add this to a code style doc, probably in CONTRIBUTING.md
2681807 to
5b713b1
Compare
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.
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 */ | |||
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 are we deleting this file (or the contents of it)? Is this a rename that I am missing?
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.
Good question! I will investigate.
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 |
Ah, I see, great.
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 |
Alright, here's version 2.0 of how we might bring unit tests over from the harperdb repo.
Things to note:
distdir. So be sure to runnpm run buildbefore you run these tests (which you do vianpm run test:unit:all)./distdir), 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 inpackage.json.*.test.*filename pattern so no further renaming was needed.Let's take our time reviewing this so we don't have to redo it again. 😅