Blocked in use#19
Open
harrylin98 wants to merge 26 commits into
Open
Conversation
### This PR adds GoogleTest (gtest) support to Valkey to enable writing modern unit tests. **Motivation**: GoogleTest provides richer assertions, test fixtures, mocking support, and improved diagnostics, helping improve test coverage and maintainability over time. **Summary**: The change is limited to test infrastructure, and existing C unit tests remain unchanged. This PR focuses only on integrating the framework and includes a small set of example tests to demonstrate usage. For more details, see `src/gtest/README.md`. --------- Signed-off-by: Harry Lin <harrylhl@amazon.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com> Co-authored-by: Harry Lin <harrylhl@amazon.com> Co-authored-by: Jim Brunner <brunnerj@amazon.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Jacob Murphy <jkmurphy@google.com>
This PR fixes the issue where changes to src files did not trigger a rebuild of libvalkey.a when rerunning `make test-gtest`. It also resolves the problem where unchanged .cpp test files were unnecessarily recompiled during make test-gtest. Now: If nothing changed: - Nothing rebuilds, no relinking occurs. If only C++ test files changed: - Both libvalkey.a and C++ test files recompile (because release.o changes) - Test executable relinks If only src code changed: - Only libvalkey.a rebuilds - C++ test files do not recompile - Test executable relinks If both changed: - Both libvalkey.a and C++ test files rebuild - Test executable relinks Signed-off-by: Harry Lin <harrylhl@amazon.com> Co-authored-by: Harry Lin <harrylhl@amazon.com>
Resolved conflicts by merging both gtest and libbacktrace features: - ci.yml: Combined gtest dependencies with libbacktrace support - Makefile: Added libbacktrace configuration alongside gtest-parallel - daily.yml: Merged g++-multilib with libbacktrace build steps All build configurations now support both USE_LIBBACKTRACE and gtest unit tests.
## Overview: This PR converted existing C unit tests to GTEST based on this introduction of GoogleTest framework valkey-io#2956 , as mentioned in valkey-io#2878 . ## Details: 1. Kept the previous test logic as much as possible, also kept all original comments from C unit tests. 2. Deleted C unit tests. 3. Changed headers: include "generated_wrappers.hpp" and remove all header files already in "generated_wrappers.hpp". 4. Used extern "C" to wrap the C include files to prevent name mangling. 5. Added Test Fixture with Setup/Teardown. ## Notes: 1. `ustime` is included in `util.h`, so deleted some duplicated `ustime`. 2. A lot of C tests include `.c` files directly to use static functions, but we don't want to copy static functions from `.c` to `.cpp`, so I added wrapper functions (prefix: `testOnly`) to `.c` to use in `.cpp` file. 3. Some C tests have shared state which is incompatible with gtest-parallel, so I changed them to isolated tests. (e.g. in `test_dict.cpp`, I made `_dict` an instance variable created fresh in SetUp() for each test, and added back the necessary setup code to each test so they can run independently in any order or in parallel). ## Testing: * GTEST: 253 tests + 32 disabled tests (285 tests in total) all pass --------- Signed-off-by: Alina Liu <liusalisa6363@gmail.com> Signed-off-by: Harry Lin <harrylhl@amazon.com> Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com> Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Co-authored-by: Harry Lin <harrylhl@amazon.com> Co-authored-by: Harry Lin <49881386+harrylin98@users.noreply.github.com> Co-authored-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>
Signed-off-by: Harry Lin <harrylhl@amazon.com> Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com> Signed-off-by: Alina Liu <liusalisa6363@gmail.com> Signed-off-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com> Co-authored-by: Harry Lin <harrylhl@amazon.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Alina Liu <liusalisa6363@gmail.com> Co-authored-by: Alina Liu <alinalq@dev-dsk-alinalq-2b-2db84246.us-west-2.amazon.com>
Signed-off-by: Harry Lin <harrylhl@amazon.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
blockedInUse
branch
from
9401485 to
fadada5
Compare
harrylin98
force-pushed
the
blockedInUse
branch
6 times, most recently
from
dc1510e to
829f5a6
Compare
Before this change, if you built `valkey-server` (e.g. `make valkey-server`) the LUA module was not built. With this change, the LUA module is now a direct a dependency of `valkey-server` - (unless `BUILD_LUA=no` is passed) Added some colors to the Lua module & Lua lib `Makefile`s to they blend nicely in the build output. Signed-off-by: Eran Ifrah <eifrah@amazon.com>
…time_t (valkey-io#3252) Addresses issue valkey-io#2350 As noted in the issue, much of the expire code uses raw long long for timestamps, which provides no semantic meaning about the unit or purpose of the value. Valkey already defines mstime_t (milliseconds) and ustime_t (microseconds) typedefs — this PR replaces bare long long declarations with the appropriate typedef wherever the value represents an expiration timestamp or time duration. This PR only fixes a small subset in the codebase, but it is an incremental step toward fully replacing the bare long long references. --------- Signed-off-by: curious-george-rk <r.ebu@gmail.com> Co-authored-by: curious-george-rk <r.ebu@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
harrylin98
force-pushed
the
blockedInUse
branch
2 times, most recently
from
c5682fb to
a48c3dd
Compare
Now we will be able to add a `run-cluster-benchmark` label to run a benchmark with cluster-mode enabled valkey-server It will use the config https://github.com/valkey-io/valkey/blob/unstable/.github/benchmark_configs/benchmark-config-arm.json modified for for cluster mode with a single clustermode enabled instance of valkey. It uses the same single instance for the benchmark as for run-benchmark. If both labels are used, they are sequential in the same concurrency group `group: ec2-al-2023-pr-benchmarking-arm64`. --------- Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
…ey-io#2746) This PR introduces a new config `cluster-message-gossip-perc` which allows an operator to modify the amount of gossip node information to be sent per ping/pong/meet message. It can be modified dynamically (Related to valkey-io#2291). The default value is 10% i.e. 10% of peer node information would be gossiped along with each ping/pong/meet packet. Users can tune this configuration, setting the value higher allows faster information dissemination whereas setting it lower would lead to direct PING messages if no information was received about a node with the `server.cluster_node_timeout/2` period. Note: the behavior for partially failed gossip nodes still remains intact where all the `pfail` nodes are part of the message for faster propagation of information and faster transition of `PFAIL` to `FAIL`. --------- Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
…o#3336) This fixes a flaky dual-channel replication integration test: https://github.com/valkey-io/valkey/actions/runs/22810251608/job/66165776198#step:8:7701 `INFO memory` field `used_memory_overhead` and `MEMORY STATS` field `overhead.total` can change during dual-channel sync if replica's pending replication buffer is still changing. This is probably more visible in slower environments. The test now collects `INFO` and `MEMORY STATS` in a single `MULTI/EXEC` on both the primary and replica, so the compared values come from the same snapshot. Passing here: https://github.com/sarthakaggarwal97/valkey/actions/runs/22864585326/job/66327772967 Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.