Skip to content
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

Migrate ziplist.c unit tests to new framework #486

Merged
merged 6 commits into from
May 22, 2024

Conversation

hallmason17
Copy link
Contributor

@hallmason17 hallmason17 commented May 10, 2024

Issue #428.

Moved the SERVER_TEST block from ziplist.c into unit tests in test_ziplist.c. I left the benchmark related tasks alone in their own test, as I am not sure what to do with them.

Some of my assertions are a little vague/useless, but I will try to refine them.

image

@hallmason17
Copy link
Contributor Author

Looks like I may also need to add a test that does pop() and another for createIntList(), since I did not add that back.

Copy link

codecov bot commented May 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.67%. Comparing base (efa8ba5) to head (240c42f).
Report is 6 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #486      +/-   ##
============================================
- Coverage     69.77%   69.67%   -0.11%     
============================================
  Files           109      109              
  Lines         61801    61801              
============================================
- Hits          43120    43057      -63     
- Misses        18681    18744      +63     
Files Coverage Δ
src/server.c 88.60% <ø> (ø)
src/ziplist.c 15.17% <ø> (ø)

... and 11 files with indirect coverage changes

@madolson madolson mentioned this pull request May 12, 2024
12 tasks
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks for contributing!

src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/unit/test_ziplist.c Outdated Show resolved Hide resolved
src/ziplist.c Show resolved Hide resolved
@hallmason17
Copy link
Contributor Author

Yikes, I'm not sure how to fix this signing issue. I tried the rebasing instructions, but the result did not look right...

@madolson
Copy link
Member

The best way is probably to do a git rebase -i HEAD~14, squash all the commit together, sign just the last one, and force push over what you have.

Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

LGTM. Ran some more tests, but it looks like it was hitting the issue that we fixed with zmalloc, so seems okay to follow up on that in mainline if it causes an issue.

@madolson madolson merged commit 7253862 into valkey-io:unstable May 22, 2024
17 checks passed
@madolson
Copy link
Member

All the updates LGTM feel free to update them further if you like.

Comment on lines +362 to +364
zl = ziplistDeleteRange(zl, 0, 1);

TEST_ASSERT(ziplistCompare(p, (unsigned char *)"foo", 3));
Copy link
Member

Choose a reason for hiding this comment

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

I believe p is getting invalidated here because of the ziplistDeleteRange. I think you need to reinitialize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like Ping may have beat me to it with #537. Sorry about that.

Copy link
Member

Choose a reason for hiding this comment

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

No worries I tried to run the daily tests before merging it, and there was another known issue so I merged it anyways. :/

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