An initial simple unit test framework

[madolson]

Here was my attempt at a unit test framework, partially ported over from redis/redis#12085, but it is slightly different.

The core idea was to take a lot of the stuff from the C unity framework and adapt it a bit here. Each file in the unit directory that starts with test_ is automatically assumed to be a test suite. Within each file, all functions that start with test_ are assumed to be a test.

See unit/README.md for details about the implementation.

Instead of compiling basically a net new binary, the way the tests are compiled is that the main valkey server is compiled as a static archive, which we then compile the individual test files against to create a new test executable. This is not all that important now, other than it makes the compilation simpler, but what it will allow us to do is overwrite functions in the archive to enable mocking for cross compilation unit functions. There are also ways to enable mocking from within the same compilation unit, but I don't know how important this is.

Tests are also written in one of two styles:

1. Including the header file and directly calling functions from the archive.
2. Importing the original file, and then calling the functions. This second approach is cool because we can call static functions. It won't mess up the archive either.

@PingXie @murphyjacob4 We discussed this as the OSS summit, so trying to get this back up somewhere.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.42%. Comparing base (8abeb79) to head (09b25c8).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #344      +/-   ##
============================================
- Coverage     68.45%   68.42%   -0.04%     
============================================
  Files           109      109              
  Lines         61671    61671              
============================================
- Hits          42217    42198      -19     
- Misses        19454    19473      +19     

Files	Coverage Δ
src/crc64.c	100.00% <ø> (ø)
src/intset.c	96.81% <ø> (ø)
src/server.c	88.12% <ø> (ø)

... and 9 files with indirect coverage changes

[murphyjacob4]

I think just the act of pulling this out into their own files will increase awareness of these tests (and hopefully encourage new additions). I personally didn't even know these existed until you pointed them out to me

For mocking - just to zoom into this real quick - my understanding is the process would look something like:

1. Write a file with a fake/mock implementation of the symbols we want to override
2. Add the file to the ENGINE_TEST_FILES in the Makefile
3. Now, the test executable will use the fake/mock implementation instead of the one in the server library

Let me know if that is right. If it is, I guess a limitation of this approach is that we can't have one test mock something and another use the real implementation (without different build targets). But I don't think it will be an issue. I hope we can steer clear of mocks as much as possible anyways.

Also - I kinda like that it will be "hard" (in comparison to other test frameworks) to mock something. I think a lot of overuse of mocks comes because it is the "easy" path.

@PingXie, you were also tagged, any thoughts? Overall I think the minimal/lightweight test framework approach is a good direction for the project and I am aligned

[madolson]

For mocking - just to zoom into this real quick - my understanding is the process would look something like:
Write a file with a fake/mock implementation of the symbols we want to override
Add the file to the ENGINE_TEST_FILES in the Makefile
Now, the test executable will use the fake/mock implementation instead of the one in the server library

Yes, it would be something like this.

Let me know if that is right. If it is, I guess a limitation of this approach is that we can't have one test mock something and another use the real implementation (without different build targets). But I don't think it will be an issue. I hope we can steer clear of mocks as much as possible anyways.

You can. You can have it point to an intermediary function which can dynamically determine which one to use. I know GMOCK supports some of that, so by default it will call the real function. You could build all functions to use the mocks, and then have GMOCK override it specifically for the test you are writing.

@PingXie, you were also tagged, any thoughts? Overall I think the minimal/lightweight test framework approach is a good direction for the project and I am aligned

Yeah. I tried to limit the amount of time I spent on this incase nobody liked it. My vision is i'll cleanup these two tests, setup the daily and CI, and then commit it and ask someone from the community to help port the rest of the tests.

[PingXie]

I like this unit test framework. Ship it!

[madolson]

@PingXie @murphyjacob4 Ok, this should be ready now. I'm a little uncertain about compatibility on all distributions though. I got some weird errors on the version of CentOS I run, but it seemed to be working on a fresh install. I've tested on centos, ubuntu, and macos. I also didn't enable it on the daily CI run, since I think that runs long enough as it is.

[PingXie]

LGTM. Thanks @madolson!

@lipzhu FYI

[hpatro]

It would be nice to capture the follow up(s) after this PR. Few things I observe are:

1. Migrate the legacy unit tests to this framework.
2. Get rid of SERVER_TEST macro.
3. Introduce mechanism to filter unit tests to run. Will allow flexibility during dev cycles.

[hpatro]

Where do we use this test flag?

[madolson]

It's used in the ziplist test, that is still on the older framework.

[murphyjacob4]

Looks good to me!

[madolson]

https://github.com/valkey-io/valkey/actions/runs/8916388262

[zuiderkwast]

@madolson Great! Please merge this so we can avoid merge conflicts when it's hanging for too long. Or merge the CRC64 optimization PR first maybe?

[madolson]

Or merge the CRC64 optimization PR first maybe?

I did CRC one first, then rebased ontop of this one.