Skip to content

Fix bin unit tests - #111

Merged
Ethan-Arrowood merged 1 commit into
add-unit-testsfrom
fix-bin-unit-tests
Jan 16, 2026
Merged

Fix bin unit tests#111
Ethan-Arrowood merged 1 commit into
add-unit-testsfrom
fix-bin-unit-tests

Conversation

@cb1kenobi

Copy link
Copy Markdown
Member

Fixes #83.

@cb1kenobi cb1kenobi added the tests Mostly focused on tests, testing infrastructure, etc. label Jan 16, 2026
@cb1kenobi
cb1kenobi requested a review from a team as a code owner January 16, 2026 20:53

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome! Thank you!

@Ethan-Arrowood
Ethan-Arrowood merged commit f63d35b into add-unit-tests Jan 16, 2026
15 of 22 checks passed
@Ethan-Arrowood
Ethan-Arrowood deleted the fix-bin-unit-tests branch January 16, 2026 23:04
@cap10morgan

cap10morgan commented Jan 23, 2026

Copy link
Copy Markdown
Contributor

I'm now realizing I probably should have done this as a part of my PR review, but I'm having a hard time getting a couple of these to pass locally:

  1. assert(100 - (stat_after.size / stat_before_compact.size) * 100 < 10);
    This fails, and I don't really understand what it's asserting now. Subtracting from 100 would give you the percentage of compaction, right? The original code without that was (statAfter.size / statBeforeCompact.size) * 100 which would be the percentage size of the "after" compared to the "before." So now wouldn't we want the assertion to be >= 90? At any rate, I'm now getting ~87% compaction. Is that indicative of a problem, or should we make this test a little more forgiving?
  2. assert(
    100 - (stat_after.size / stat_before_compact.size) * 100 < 10,
    'after size ' + stat_after.size + ' should be' + ' much less than before size ' + stat_before_compact.size
    );
    This fails for (I think) similar reasons. And the test failure says: after size 278528 should be much less than before size 2211840

Help me @cb1kenobi, you're my only hope!

@cb1kenobi

Copy link
Copy Markdown
Member Author

@cap10morgan This test was failing for me too in both repos. I figured it was a bad test. In theory the after size is less than the before size which would result in a ratio < 1.0. I assumed 10 was the percentage compaction, so you are spot on.

Personally, I would assert the after < before and call it a day. I don't know how consistent the compaction is and any compaction should prove compaction worked, right?

@cap10morgan

Copy link
Copy Markdown
Contributor

@cap10morgan This test was failing for me too in both repos. I figured it was a bad test. In theory the after size is less than the before size which would result in a ratio < 1.0. I assumed 10 was the percentage compaction, so you are spot on.

Personally, I would assert the after < before and call it a day. I don't know how consistent the compaction is and any compaction should prove compaction worked, right?

Yeah, that all makes sense and I agree the simpler the better. I might do that in a future PR. I want to make the minimum changes to get tests passing and migrated into this repo first. You can see where I'm starting in #117.

@cb1kenobi

Copy link
Copy Markdown
Member Author

@cap10morgan I'm still a little puzzled why the tests passed on my Mac. Maybe the cold weather?

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

Labels

tests Mostly focused on tests, testing infrastructure, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants