Skip to content

Add more test coverage for Tsavorite primitives #1121

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Mar 21, 2025

Add little more SpanByte and its associated comparison logic unit test coverage. Also contains some Tsavorite test code housekeeping (mostly doing switching to simpler UTF-8 logic instead of UTF-16).

Follow up work

  • There could always be more SpanByte specific unit tests in Tsavorite.test

@badrishc
Copy link
Collaborator

badrishc commented Mar 21, 2025

This may still be needed, as the MetadataSize check is different from Length check. A SpanByte may have an 8 byte optional header used for storing logical record expiration time (a bit tells us whether there is such a header). The MetadataSize check ensures that both left and right have such a metadata header. MetadataSize will be either 8 or 0, depending on this bit.

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Mar 21, 2025

Ah, thanks for explanation. I should have looked where that getter leads to before jumping to conclusion. 🤦‍♂️

I probably should pivot this PR to add tests to catch this detail, that the KeyComparer must be able to distinguish SpanBytes if their MetadataSizes are different. Bit spooky that the CI passed

@PaulusParssinen PaulusParssinen marked this pull request as draft March 21, 2025 18:24
@PaulusParssinen PaulusParssinen changed the title Remove repeat length check from SpanByteComparer.Equals Add more test coverage to for Tsavorite primitives Mar 21, 2025
@PaulusParssinen PaulusParssinen changed the title Add more test coverage to for Tsavorite primitives Add more test coverage for Tsavorite primitives Mar 21, 2025
Comment on lines +72 to +81
[Test]
public void Equals_ReturnsTrue_ForEmptyAndInvalid()
{
var invalid = new SpanByte();
invalid.Invalid = true;

var valid = new SpanByte();

ClassicAssert.IsTrue(SpanByteComparer.Instance.Equals(ref invalid, ref valid));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought to just show this here and ask whether it is intentional or leave it as "undefined" behaviour and we should not try make any contract in the tests?

@PaulusParssinen PaulusParssinen marked this pull request as ready for review May 19, 2025 14:27
@PaulusParssinen
Copy link
Contributor Author

Seems like unrelated flaky test failure. There is probably more to do in Tsavorite tests but this will do as a start. Suggestions for any interesting or important test cases to still implement in this PR are welcome 👍

@badrishc
Copy link
Collaborator

Ted's large PR at #1186 might clash with this one. We were hoping to get that in without too many more clashes. Any chance this could be revised to work off of that branch?

@PaulusParssinen
Copy link
Contributor Author

Ted's large PR at #1186 might clash with this one. We were hoping to get that in without too many more clashes. Any chance this could be revised to work off of that branch?

Sounds good, I can revisit this once that merges 👍

@PaulusParssinen PaulusParssinen marked this pull request as draft May 19, 2025 16:42
@badrishc
Copy link
Collaborator

Thanks @PaulusParssinen , would also appreciate if you can review that PR #1186 (fine to review the draft, as the pending changes right now are focused on larger-than-memory modifications) -- as it makes a lot of the improvements that have been suggested by you and other .NET experts over the last year.

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.

2 participants