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 quicklist unit test to new framework #515

Open
wants to merge 8 commits into
base: unstable
Choose a base branch
from

Conversation

artikell
Copy link
Contributor

It seems that the quicklist is a very troublesome change, and currently PR has attempted to complete the migration of the quicklist. There are several modification points involved:

  • Removed static tag from quicklist
  • Added TestMain for initializing test data
  • Added a declaration to quicklist. h
  • There are fewer logs compared to before, mainly the logs brought by TEST and TEST DECR
  • Default change to attempted_compress

Although there are many points that need to be discussed.

Copy link

codecov bot commented May 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.68%. Comparing base (0d7b234) to head (3fdb2f8).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #515      +/-   ##
============================================
- Coverage     70.70%   70.68%   -0.02%     
============================================
  Files           114      114              
  Lines         63157    63159       +2     
============================================
- Hits          44654    44647       -7     
- Misses        18503    18512       +9     
Files with missing lines Coverage Δ
src/quicklist.c 84.14% <ø> (-0.22%) ⬇️
src/server.c 87.70% <ø> (ø)

... and 14 files with indirect coverage changes

@artikell artikell force-pushed the quicklist_test_framework branch 4 times, most recently from 2a6f11e to 9790186 Compare May 25, 2024 07:17
Signed-off-by: artikell <739609084@qq.com>
Signed-off-by: artikell <739609084@qq.com>
Signed-off-by: artikell <739609084@qq.com>
Signed-off-by: artikell <739609084@qq.com>
Signed-off-by: artikell <739609084@qq.com>
…it test

Signed-off-by: artikell <739609084@qq.com>
@artikell
Copy link
Contributor Author

@madolson I need your help to review it. I have broken it down into multiple commitments. Used to assist with code review.
There are several key points involved in this:

  • I tried to include quicklist. c
  • Used python script to replace function
  • Continuing to use err to determine if there is an error

@artikell
Copy link
Contributor Author

artikell commented Oct 9, 2024

@enjoy-binbin @madolson It seems that there are only SERVER_TEST left in quicklist, I want to push this forward. There are more cases in this part. Do we want to merge in batches, or is it more appropriate to merge with one mr?

@enjoy-binbin
Copy link
Member

enjoy-binbin commented Oct 9, 2024

I think one PR should be enough, there is no need for more batches, please go ahead, thanks

Signed-off-by: skyfirelee <739609084@qq.com>
Signed-off-by: artikell <739609084@qq.com>
@artikell
Copy link
Contributor Author

artikell commented Nov 2, 2024

I think one PR should be enough, there is no need for more batches, please go ahead, thanks

The conflict has been resolved. Could you kindly review it? Thank you

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