More and better unit tests by cap10morgan · Pull Request #15 · HarperFast/harper

cap10morgan · 2025-10-29T21:28:32Z

This started as me just experimenting with how much work it would be to migrate the resources unit tests (😅). Pretty quickly I ran into issues of the tests not being hermetic; both with respect to each other and with respect to the broader system.

So I dove back down that rabbit hole, but this time I hit pay dirt.

These unit tests all pass by themselves and together, and don't care one lick what is or isn't in your home directory (at least the Harper-y stuff; ~/.harperdb, ~/hdb, etc.).

This PR introduces a new unitTests/testUtils.ts module that defines two important functions:

createTestSandbox - this creates an environment in which tables can be created and used, but does not need nor use any existing Harper installation; it should be called in a before hook (or a beforeEach if you find it really necessary)
cleanupTestSandbox - this cleans up after createTestSandbox and should be called in the after hook (or afterEach if you are calling createTestSandbox in beforeEach). This one is async, so if it isn't the only thing you're passing to the hook (e.g. after(cleanupTestSandbox);), be sure to await it so it finishes its job before other tests are run.

...and the need to even have one at all

unitTests/resources/create.test.ts

For some reason the unit tests stopped working for me today w/o this

...so it doesn't pollute other tests

Use lambdas & remove unnecessary asyncs

cap10morgan · 2025-10-31T15:35:28Z

We found some additional isolation work is needed from config / data in the home directory.

The prior approach broke the ability to specify a subset of tests on the command line

It's pretty flaky, but we're working on it

The before hook is failing pretty often

Ethan-Arrowood

Everything looks great. One code nit pick and a necessary change in the package.json scripts. nice work!

Ethan-Arrowood · 2025-10-31T22:23:57Z

unitTests/testUtils.ts

Not sure how many tables there'd be but you can make this significantly faster by doing:

Suggested change

for (const Table of envs) {

try {

await Table.delete();

// eslint-disable-next-line no-empty

} catch (err) {}

}

await Promise.all(envs.map(table => table.delete())).catch();

This is old code from the other repo's test_utils.js so I just copied it over. But sounds like a good change 👍🏻

Done in e692c5e

Ethan-Arrowood · 2025-10-31T22:35:26Z

package.json

Remove this.

The point of splitting test:unit and test:unit:all is so that you can run npm run test:unit <specific-unit-test-path> in order to run a singular test file. The test:unit:all passes in the unitTests directory to test:unit so you can run all the unit tests.

Hmm... so now with this change running npm run test:unit gives you an error that there are no tests. I wasn't aware that test:unit:all was added so this was surprising and seemed broken. But this is all pretty new so no big deal and now I know. I do wish there were a reasonable way to output a more helpful error message, or even just have this default to "all" when nothing is specified, but I can't think of one.

Done in 9084d1e

Agreed; I wish there was a better default or something. I couldn't think of a reasonable solution either that didn't involve like executing it via a mini cli script.

kriszyp · 2025-11-03T20:38:01Z

server/http.ts

-			maxHeaderSize: env.get(terms.CONFIG_PARAMS.HTTP_MAXHEADERSIZE),
 		};
+		if (keepAliveTimeout) {
+			options['keepAliveTimeout'] = Number(keepAliveTimeout);


eslint lets you get away with using strings in brackets here? Why?

Should it not? IIRC I did this instead of actually specifying an interface for the options. But I can certainly go back and do that instead.

This is a workaround for violating TypeScript types? TIL!

kriszyp

Why are we converting unit tests to TypeScript? If we want our code to be used by TypeScript and JavaScript, isn't JavaScript the broader language that would provide better unit testing coverage?

kriszyp

I thought we had been using a convention of module-test.js for test names. Where did we decide to change this?

cap10morgan · 2025-11-03T20:47:31Z

I thought we had been using a convention of module-test.js for test names. Where did we decide to change this?

I wasn't aware we had a convention, and had already run into both the one I used here and the one you describe in the tests that had been brought over from the old repo. I ran across something that said .test.[tj]s was the most common convention in the JS community so picked that one. But if we have a different one we've already decided on, I'm happy to change the filenames to conform to it.

I was planning to write up some conventions for tests in this repo (in a contributing doc or similar), but it sounds like there are some already written up I need to (re-)familiarize myself with. Do you have a link?

kriszyp · 2025-11-03T20:43:35Z

unitTests/resources/validation.test.ts

+ *
+ * - WSM 2025-10-31
+ */
+describe.skip('Types Validation', () => {


Do we have a follow-up ticket for this? This seemed to be a pretty reliable test before.

I'll create it along with others for the other skipped tests.

kriszyp · 2025-11-03T20:45:49Z

utility/common_utils.js

 function getHomeDir() {
-	let homeDir = undefined;
+	// for hermetic tests
+	if (process.env.OVERRIDE_HOME_DIR) {


Does this function differently than process.env.ROOTPATH?

Does this function differently than process.env.ROOTPATH?

Yes, but that seems to work just as well in its place, so I'll issue a followup PR that switches over. Thanks!

kriszyp · 2025-11-03T20:47:34Z

server/http.ts

-			maxHeaderSize: env.get(terms.CONFIG_PARAMS.HTTP_MAXHEADERSIZE),
 		};
+		if (keepAliveTimeout) {
+			options['keepAliveTimeout'] = Number(keepAliveTimeout);


This is a workaround for violating TypeScript types? TIL!

cap10morgan · 2025-11-03T20:55:31Z

Why are we converting unit tests to TypeScript? If we want our code to be used by TypeScript and JavaScript, isn't JavaScript the broader language that would provide better unit testing coverage?

I think having them be TypeScript gives us (optional) tools to improve the quality of our tests just like it does for the code itself. And I have a hard time imaging test code that passes in TS but would allow a valid JS equivalent to fail (it's entirely possible that this is a failure of my imagination, however!). I'd be curious to hear what others in the community think about this, though.

It also allows us to use the @ path alias in the test imports, but that's probably not sufficient justification to use TS all on its own.

kriszyp · 2025-11-03T20:56:43Z

Do you have a link?

https://github.com/HarperFast/harperdb/tree/main/unitTests has hundred(s?) of test cases with module-test.js naming convention.

cap10morgan · 2025-11-03T21:00:22Z

Why are we converting unit tests to TypeScript? If we want our code to be used by TypeScript and JavaScript, isn't JavaScript the broader language that would provide better unit testing coverage?

And I have a hard time imaging test code that passes in TS but would allow a valid JS equivalent to fail (it's entirely possible that this is a failure of my imagination, however!). I'd be curious to hear what others in the community think about this, though.

But perhaps you're thinking of valid JS code that isn't valid TS code that we'd want to test for expected behavior? I guess we'd either have to sprinkle around directives to expect TS errors here and there in some test code, or, like you said, keep the tests written in JS. I think I prefer the former, but it's not a strongly-held opinion. Again I would love to hear what others think here.

cap10morgan · 2025-11-03T21:04:44Z

Do you have a link?

https://github.com/HarperFast/harperdb/tree/main/unitTests has hundred(s?) of test cases with module-test.js naming convention.

That's true and fair. I had started with bringing over the components unit tests, which were all thingy.test.js and then brought over these so conformed them to those after reading that it was the more common convention in the broader JS community (and the fact that this repo is kind of a "reset" along these lines).

I am happy to use either convention, though!

kriszyp · 2025-11-04T15:55:44Z

I don't see how this properly going to track with cherry-picking, since the tests were copied in with different names and there is no rename history. I think this PR needs to be reverted and submitting again with direct filename preservation (like unitTests/resources/blob-test.js), so we properly keep our repositories in sync.

cap10morgan · 2025-11-04T16:18:17Z

I don't see how this properly going to track with cherry-picking, since the tests were copied in with different names and there is no rename history. I think this PR needs to be reverted and submitting again with direct filename preservation (like unitTests/resources/blob-test.js), so we properly keep our repositories in sync.

Makes sense. Maybe if we decide on a naming convention (because there would already be two different ones in here with just these two categories of unit tests), we could bring the files in w/ their existing names, commit, rename, commit, and then adapt them to this repo?

Thoughts, @Ethan-Arrowood?

Ethan-Arrowood · 2025-11-04T16:22:34Z

Yeah so git is actually pretty smart as long as you do the rename through a commit as you described Wes.

Seeing some of Kris' comments now I think redoing this PR would be best.

Since this was merged using a merge commit, adding many individual commits, I think we may just want to reset main branch to before this PR was merged and try again (rather than adding a "revert commit"). This should help us clean up main branch history so its easier for me to sync later.

I'll confirm this procedure on today's standup call

Ethan-Arrowood · 2025-11-04T18:05:11Z

For public record:

We confirmed on our eng call today to reset the main branch and redo this PR. I'll be completing this shortly and then posting about the main branch update across relevant channels promptly.

cap10morgan and others added 7 commits October 29, 2025 15:21

Various fixes for unit tests to run w/o install

02b6cfb

Isolate unit tests from Harper installation

a45a0bd

...and the need to even have one at all

Add some more unit tests

d3fd87e

Stop installing Harper in the unit tests CI workflow

c928d2d

Merge branch 'main' into tests/more-and-better-unit-tests

0c08d9c

Commit prettier fixes

50639e5

Try to fix create.test under Node 20

a66e386

cap10morgan marked this pull request as ready for review October 29, 2025 21:42

cap10morgan requested a review from a team October 29, 2025 21:42

cap10morgan requested a review from a team as a code owner October 29, 2025 21:42

heskew reviewed Oct 30, 2025

View reviewed changes

unitTests/resources/create.test.ts Show resolved Hide resolved

cap10morgan added 7 commits October 30, 2025 10:45

Remove unhelpful attempt at speeding up a test

b6117f3

Add a spec to .mocharc.json

51e7995

For some reason the unit tests stopped working for me today w/o this

await the test sandbox cleanup when necessary

e9e1cd9

...so it doesn't pollute other tests

Add a test sandbox to the vector index tests

eb58402

Commit prettier fixes

8ecc299

Remove commented-out tests that were an experiment

18ead7f

Clean up validation tests

0bc5034

Use lambdas & remove unnecessary asyncs

cap10morgan marked this pull request as draft October 31, 2025 15:35

cap10morgan added 5 commits October 31, 2025 10:55

Change how we tell mocha which tests to run

1793189

The prior approach broke the ability to specify a subset of tests on the command line

Isolate test sandboxes even when Harper is installed

5ebddbd

Skip blob save & delete test for now

3a77493

It's pretty flaky, but we're working on it

Couple small cleanups / fixes

3b1587e

Skip validation unit tests for now

862db6c

The before hook is failing pretty often

cap10morgan marked this pull request as ready for review October 31, 2025 19:10

Ethan-Arrowood requested changes Oct 31, 2025

View reviewed changes

cap10morgan added 2 commits October 31, 2025 16:45

Make test table deletion code faster

e692c5e

Remove unitTests arg to test:unit npm script

9084d1e

Ethan-Arrowood approved these changes Nov 3, 2025

View reviewed changes

heskew approved these changes Nov 3, 2025

View reviewed changes

cap10morgan merged commit fb71d95 into main Nov 3, 2025
7 checks passed

cap10morgan deleted the tests/more-and-better-unit-tests branch November 3, 2025 17:44

kriszyp reviewed Nov 3, 2025

View reviewed changes

cap10morgan mentioned this pull request Nov 3, 2025

Followup to PR #15 #20

Closed

Ethan-Arrowood mentioned this pull request Nov 4, 2025

Testing Standards: Integration vs Unit Tests, TypeScript vs JavaScript, CJS vs ESM #22

Open

5 tasks

cap10morgan mentioned this pull request Jan 6, 2026

Migrate component unit tests #66

Merged

-			for (const Table of envs) {
-				try {
-					await Table.delete();
-					// eslint-disable-next-line no-empty
-				} catch (err) {}
-			}
+			await Promise.all(envs.map(table => table.delete())).catch();

Conversation

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

Uh oh!

Uh oh!

Uh oh!

cap10morgan commented Oct 31, 2025

Uh oh!

Ethan-Arrowood 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!

cap10morgan Oct 31, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

kriszyp left a comment

Choose a reason for hiding this comment

Uh oh!

kriszyp left a comment

Choose a reason for hiding this comment

Uh oh!

cap10morgan commented Nov 3, 2025

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!

cap10morgan commented Nov 3, 2025

Uh oh!

kriszyp commented Nov 3, 2025

Uh oh!

cap10morgan commented Nov 3, 2025

Uh oh!

cap10morgan commented Nov 3, 2025

Uh oh!

kriszyp commented Nov 4, 2025

Uh oh!

cap10morgan commented Nov 4, 2025

Uh oh!

Ethan-Arrowood commented Nov 4, 2025

Uh oh!

Ethan-Arrowood commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

cap10morgan commented Oct 29, 2025 •

edited

Loading

cap10morgan Oct 31, 2025 •

edited

Loading