valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Migrate quicklist unit test to new framework

Open artikell opened this issue 1 year ago • 4 comments

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.

artikell avatar May 19 '24 15:05 artikell

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.69%. Comparing base (0d7b234) to head (76997ad). Report is 19 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #515      +/-   ##
============================================
- Coverage     70.70%   70.69%   -0.01%     
============================================
  Files           114      114              
  Lines         63157    63163       +6     
============================================
- Hits          44654    44652       -2     
- Misses        18503    18511       +8     
Files with missing lines Coverage Δ
src/quicklist.c 84.40% <ø> (+0.04%) :arrow_up:
src/server.c 87.69% <ø> (-0.02%) :arrow_down:

... and 18 files with indirect coverage changes

codecov[bot] avatar May 19 '24 15:05 codecov[bot]

@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 avatar May 25 '24 16:05 artikell

@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?

artikell avatar Oct 09 '24 03:10 artikell

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

enjoy-binbin avatar Oct 09 '24 03:10 enjoy-binbin

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

artikell avatar Nov 02 '24 02:11 artikell

CI report some failure, please take a look: https://github.com/valkey-io/valkey/actions/runs/11716457128/job/32634549755?pr=515#step:4:560

sed '/Everything below this line/,$d' fmtargs.h > fmtargs.h.tmp
/usr/bin/python3 ../utils/generate-fmtargs.py >> fmtargs.h.tmp
mv fmtargs.h.tmp fmtargs.h
    CC server.o
server.c:6799:19: error: ISO C forbids empty initializer braces [-Werror=pedantic]
 6799 | } serverTests[] = {};
      |                   ^
server.c:6799:3: error: zero or negative size array ‘serverTests’
 6799 | } serverTests[] = {};
      |   ^~~~~~~~~~~

enjoy-binbin avatar Nov 07 '24 05:11 enjoy-binbin

CI report some failure, please take a look: https://github.com/valkey-io/valkey/actions/runs/11716457128/job/32634549755?pr=515#step:4:560

sed '/Everything below this line/,$d' fmtargs.h > fmtargs.h.tmp
/usr/bin/python3 ../utils/generate-fmtargs.py >> fmtargs.h.tmp
mv fmtargs.h.tmp fmtargs.h
    CC server.o
server.c:6799:19: error: ISO C forbids empty initializer braces [-Werror=pedantic]
 6799 | } serverTests[] = {};
      |                   ^
server.c:6799:3: error: zero or negative size array ‘serverTests’
 6799 | } serverTests[] = {};
      |   ^~~~~~~~~~~

ok,Let me clean this up together

artikell avatar Nov 07 '24 07:11 artikell

i see daily.ci is still have some valkey-server test all, please also remove it, and also use the new ./src/valkey-unit-tests to replace it. please also remove all DSERVER_TEST

enjoy-binbin avatar Nov 11 '24 02:11 enjoy-binbin

i see daily.ci is still have some valkey-server test all, please also remove it, and also use the new ./src/valkey-unit-tests to replace it. please also remove all DSERVER_TEST

@enjoy-binbin Is there a way to retry a process alone? It always seems to get stuck in strange places.

artikell avatar Nov 12 '24 02:11 artikell

I see the top comment is outdated, could you please update the top comment to reflect the changes? And then i believe we are good to go.

enjoy-binbin avatar Nov 13 '24 10:11 enjoy-binbin