ci(linux): bring up redis-server and mysql-server for unittests#3323
Conversation
The clang-unittest and clang-unittest-asan jobs run the full unit test suite via test/run_tests.sh, which includes backend integration tests (e.g. brpc_redis_unittest) that fork a real server when its binary is present and otherwise silently short-circuit to a passing result. Since CI never installed those servers, the redis backend tests reported PASSED while doing nothing (7 of 14 RedisTest cases skip-as-pass). Install redis-server and mysql-server before running the tests in both unittest jobs so these backend tests execute against a live server. The binaries are added only in the unittest jobs, not in the shared install-essential-dependencies action used by compile-only jobs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The failing
|
brpc_redis_unittest forks a real redis-server and waits a fixed 50ms before connecting; under ASan redis starts too slowly, causing flaky connection-refused. Skip RedisTest.* in the ASan job only (still covered by clang-unittest). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CI is green on the latest commit. Full Build and Test on Linux run passes — all jobs, including Ready for review. |
There was a problem hiding this comment.
Pull request overview
Updates the Linux GitHub Actions unittest jobs to ensure Redis/MySQL-dependent tests are exercised in CI (instead of being skipped when servers/binaries are missing).
Changes:
- Install
redis-server(and currentlymysql-server) in theclang-unittestjob before runningtest/run_tests.sh. - Do the same for
clang-unittest-asan, and additionally apply aGTEST_FILTERto avoid Redis-related ASan flakiness.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: install redis-server and mysql-server | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y redis-server mysql-server | ||
| redis-server --version | ||
| mysqld --version |
There was a problem hiding this comment.
Expected. mysql-server is installed intentionally ahead of the in-progress MySQL protocol support (#2093); the client tests for it land in a follow-up, and pre-provisioning the server here avoids a second CI change when they do. The extra install time is small and the step is green in every job.
| - name: install redis-server and mysql-server | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y redis-server mysql-server | ||
| redis-server --version | ||
| mysqld --version |
There was a problem hiding this comment.
Same as the clang-unittest job above — mysql-server is pre-installed intentionally for the upcoming MySQL protocol tests (#2093). Expected, not extraneous.
| # brpc_redis_unittest forks a real redis-server and waits a fixed 50ms before | ||
| # connecting; under ASan redis starts too slowly, so the redis client tests are | ||
| # flaky here (connection refused). Skip them in ASan; they run in clang-unittest. | ||
| GTEST_FILTER='-RedisTest.*' sh ./run_tests.sh |
There was a problem hiding this comment.
Applied. The ASan filter now excludes only the five client tests that connect to the forked redis-server — RedisTest.sanity, keys_with_spaces, incr_and_decr, by_components, auth. The pure parsing/service cases (command_parser, redis_reply_codec, memory_allocation_limits, server_*) still run under ASan, so coverage of that logic is preserved.
| cat config.mk | ||
| cd test | ||
| make -j ${{env.proc_num}} | ||
| - name: install redis-server and mysql-server |
There was a problem hiding this comment.
You can refer to install-essential-dependencies and add an action to install the Redis server and MySQL server.
There was a problem hiding this comment.
Done — moved the redis-server / mysql-server install into the install-essential-dependencies composite action and removed the per-job install steps from clang-unittest and clang-unittest-asan. Every job that pulls in those dependencies now brings up both servers. Re-validated on my fork: all Linux jobs green (https://github.com/rajvarun77/brpc/actions/runs/26858989158).
| # flaky here (connection refused). Skip them in ASan; they run in clang-unittest. | ||
| GTEST_FILTER='-RedisTest.*' sh ./run_tests.sh | ||
|
|
||
| clang-unittest-bazel-with-babylon-and-new-pb: |
There was a problem hiding this comment.
Do clang-unittest-bazel-with-babylon-and-new-pb, clang-unittest-with-bazel and gcc-unittest-with-bazel also require Redis server to be installed?
There was a problem hiding this comment.
Good question. gcc-unittest-with-bazel, clang-unittest-with-bazel and clang-unittest-bazel-with-babylon-and-new-pb don't use the install-essential-dependencies action, so this change doesn't install Redis/MySQL in them. The Redis client tests there continue to self-skip when redis-server is absent — the same behaviour as before this PR. If you'd like those bazel jobs to exercise the live-server tests as well, I'm happy to add it in a follow-up.
There was a problem hiding this comment.
Done — addressed in the latest commit. gcc-unittest-with-bazel, clang-unittest-with-bazel, and clang-unittest-bazel-with-babylon-and-new-pb now pull in the install-essential-dependencies action, and brpc_redis_unittest is tagged external + local so it runs unsandboxed (where the PATH-located redis-server is visible and loopback works) and re-runs rather than serving a cached pass.
The Redis integration tests now genuinely fork redis-server and run under Bazel — confirmed from the logs of the latest green Build and Test on Linux run: the forked redis-server reaches Ready to accept connections, and RedisTest.sanity / keys_with_spaces / incr_and_decr / by_components / auth all pass (not skipped, not cached).
…-dependencies Move the redis-server/mysql-server install out of the individual clang-unittest and clang-unittest-asan jobs and into the shared install-essential-dependencies composite action, so every job that installs dependencies has the servers available (and the unittest jobs no longer carry a bespoke install step). Under ASan the redis integration tests (sanity, keys_with_spaces, incr_and_decr, by_components, auth) fork a real redis-server and connect after a fixed 50ms wait; redis starts too slowly there and they flake with connection refused. Filter just those out under ASan -- the redis codec/server tests still run, and the full suite runs in clang-unittest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the review feedback:
Re-validated end-to-end on my fork — all Linux jobs green, including |
Log verification: Redis integration tests run in
|
The bazel unittest jobs ran //test/... without redis-server/mysql-server installed, so brpc_redis_unittest forked nothing and its integration cases (sanity/keys_with_spaces/incr_and_decr/by_components/auth) early-returned as passes -- green but vacuous. - Install redis-server/mysql-server in the (non-ASan) bazel test jobs via the shared install-essential-dependencies action (the same one the make jobs use), so the servers are on PATH for the forked tests. - Tag brpc_redis_unittest "external" + "local" so bazel always re-runs it (never serves a cached skip) and runs it outside the sandbox where the PATH-located redis-server is visible and loopback works. Threaded through generate_unittests via a new per_test_tags arg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All review feedback is addressed and CI is green — ready for another look.
Thanks @chenBright for the review. |
|
Thanks for the review, @chenBright! All comments are addressed and CI is green on the LGTM'd head |
…sqld The MySQL integration tests were not exercising a live server in CI: - Merge master to pick up apache#3323, which installs mysql-server in the install-essential-dependencies action used by the unittest lanes, so the mysqld binary is present for the self-spawning test/mysql/* suites. - Delete the legacy test/brpc_mysql_unittest.cpp: it hardcoded an external host (db4free.net) with embedded credentials and asserted on connect failure (no skip), so it timed out and hard-failed in CI. The test/mysql/*_integration suites supersede it, spawning a throwaway local mysqld and GTEST_SKIP-ing when none is available (the redis precedent). - Build the test/mysql/* suites in the make lane: extend test/Makefile's source glob and add a mysql/-prefixed link rule; add the subdir to run_tests.sh with nullglob so it runs them. - Bazel: build one cc_test per mysql test file via generate_unittests instead of globbing them all into a single brpc_mysql_test target, which duplicated main() and the FLAGS_mysql_* definitions (ld: duplicate symbol). Drop the per-file main()s so every suite relies on gtest_main, matching brpc_redis_unittest; tag the server-spawning suites external+local so bazel runs them unsandboxed with the real mysqld. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ements) on clean-room auth (#3330) * feat(mysql): clean-room MySQL authentication codec Clean-room implementation of the MySQL connection-phase authentication handshake, derived from the public MySQL protocol documentation with no GPL lineage: mysql_native_password and caching_sha2_password scrambles, HandshakeV10/ HandshakeResponse41 codec, and length-encoded integer/string plus packet-header wire helpers. Handles the lenenc NULL (0xFB) marker and rejects an oversize auth_response. * feat(mysql): full MySQL text protocol with transactions and prepared statements Port the MySQL protocol client (issue #2093) onto the clean-room auth codec and protobuf 3.21 (NonreflectableMessage): COM_QUERY text protocol, interactive transactions via connection affinity, and prepared statements. Wire caching_sha2_password (fast-auth, full-auth RSA, and secure-transport cleartext) into the live client. Fix the lenenc 9-byte length marker in pack_encode_length (0xFD -> 0xFE) per the MySQL protocol spec. * test(mysql): clean-room integration tests + prepared-stmt error fix + Controller cleanup - Add clean-room integration tests (transactions, prepared statements, pooled connection concurrency, connection-type) run against a self-spawned mysqld. - Fix: a failed COM_STMT_PREPARE now returns the ERR packet to the caller and keeps the connection alive, instead of closing the socket. - Warn when a prepared statement runs on a 'short' connection (re-prepares on every execute; prefer 'pooled'). - Replace Controller's mysql-specific _mysql_stmt with a generic opaque per-RPC slot so no protocol type leaks onto the shared Controller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(mysql): consolidate sources under policy/mysql/; drop inherited images Move all MySQL sources (mysql.*, mysql_command/reply/common/transaction/ statement*, mysql_protocol.*, mysql_authenticator.*) into src/brpc/policy/mysql/ alongside the clean-room auth codec; update all includes, build globs, and install rules. Remove three benchmark images inherited from the #2093 port and the doc section referencing them. No behavior change: full build green; all 19 mysql unit/integration tests pass; a 30-case standalone end-to-end run was independently verified against mysqld. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mysql): address code-review findings (binary protocol, auth, edge cases) + ASF headers - Binary DATETIME/TIME: gate the microsecond bytes on the packet length, not the column's declared decimals (over-read / result-set desync). - COM_STMT_SEND_LONG_DATA: frame stmt_id/param_id inside the packet; fix chunk offset. - COM_STMT_EXECUTE: emit the trailing 0-length packet for 16MiB-aligned payloads. - OK/EOF status & warnings: decode via mysql_uint2korr (big-endian safe). - Row NULL-bitmap: arena-allocate instead of a stack VLA; cap column_count. - Auth: bounds-check the parsed auth string; size-bound StringPiece uses. - Prepared stmt: prune stale per-socket stmt_id map entries; count only real '?' placeholders (skip quotes/comments). - MysqlResponse::Clear and MysqlRequest copy/Swap: reset/copy all members. - Controller::ResetPods: release _bind_sock on controller reuse. - Standardize mysql file license headers to the ASF form; clarify auth comment. All 19 unit/integration tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mysql): address review findings — license header, flag typo, header-global, doc/comment drift - example/mysql_c++/mysql_go_press.go: add ASF Apache-2.0 license header (the last file failing the License Check; other 6 already had headers). - mysql_common.{h,cpp}: move the MysqlCollations map definition out of the header into the .cpp behind an `extern` declaration, so each translation unit no longer gets its own copy (C++11-safe; avoids the header-defined global flagged in review). - mysql_statement.{cpp,inl.h}: rename the misspelled gflag mysql_statment_map_size -> mysql_statement_map_size (user-facing name). - docs/cn/mysql_client.md: prepared statements ARE supported now — drop the stale "不支持Prepared statement". - brpc_mysql_connection_type_unittest.cpp: rewrite the stale header comment that described a removed "MustError" test; the prepared-statement path now transparently re-prepares under CONNECTION_TYPE_SHORT and succeeds, matching the actual PreparedStatementUnderShortRePreparesAndSucceeds test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mysql): use the standard ASF license header on new example/test files The new MySQL example sources and brpc_mysql_unittest.cpp carried the old "Copyright (c) Baidu, Inc." Apache-2.0 header, which skywalking-eyes (the repo's License Check, configured copyright-owner = Apache Software Foundation) does not accept — so all 7 files failed the gate. Replace with the canonical ASF header used by the other 549 sources in the tree, and drop the stale Baidu copyright/date attribution lines. Verified locally with `license-eye -c .licenserc.yaml header check`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(mysql): add diagnostic error logs on silent failure paths Extend the failure-path logging from the auth codec to the full client protocol. Parsers and request/response handlers returned false/0/nullptr silently on malformed wire data, truncated packets, bad state, or missing prepared statements, giving no clue why a query/connection failed. Add a LOG(ERROR) at each silent failure path naming the function and the concrete cause. - mysql_auth_handshake/packet.cpp: handshake + lenenc codec failure paths (truncated <field>, pre-4.1 server, reserved 0xFF marker, length mismatch). - mysql.cpp: request command/param guards. - mysql_reply.cpp: result-set parse (column/row/error packet truncation, arena alloc failure, bad binary-row header). - mysql_protocol.cpp: serialize/process type + serialization failures. - mysql_statement.cpp: statement-id lookup misses (not-prepared / stale conn). Logic unchanged — logs inserted only. Normal control-flow returns (need-more-data, lenenc NULL 0xFB, short-connection no-cache) are not logged. mysql_authenticator.cpp and mysql_transaction.cpp already logged every failure path and were left unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(mysql): demote non-fatal diagnostic logs to LOG(WARNING) Tune the severity of the failure-path logs. LOG(ERROR) is reserved for total failures — handshake/connection corruption, authentication failure, out-of-memory, and fundamental request/response type or serialization misuse. Operational and statement-data failures are demoted to LOG(WARNING): - mysql_auth_packet.cpp: lenenc int/string/header decode failures (these fire during normal resultset parsing). - mysql.cpp: request command/param API guards. - mysql_reply.cpp: column/row/field/ERR-packet truncation (statement data). The arena out-of-memory and Auth::Parse auth-plugin failures stay ERROR. - mysql_statement.cpp: prepared-statement-id lookup misses (recoverable, trigger a re-prepare). handshake parsing and the protocol serialize/process type checks remain LOG(ERROR). Severity-only change; messages and logic unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(test): make AuthCase a C++11 aggregate so push_back{...} compiles CI compiles unittests with -std=c++0x (C++11), where a class with a default member initializer is not an aggregate, so g_auth_cases.push_back({...}) fails to compile. Drop the default member initializer on AuthCase::use_ssl; all init sites pass it explicitly. Mirrors the same fix on the #3310 codec branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(mysql,ci): run integration tests against a real CI-provisioned mysqld The MySQL integration tests were not exercising a live server in CI: - Merge master to pick up #3323, which installs mysql-server in the install-essential-dependencies action used by the unittest lanes, so the mysqld binary is present for the self-spawning test/mysql/* suites. - Delete the legacy test/brpc_mysql_unittest.cpp: it hardcoded an external host (db4free.net) with embedded credentials and asserted on connect failure (no skip), so it timed out and hard-failed in CI. The test/mysql/*_integration suites supersede it, spawning a throwaway local mysqld and GTEST_SKIP-ing when none is available (the redis precedent). - Build the test/mysql/* suites in the make lane: extend test/Makefile's source glob and add a mysql/-prefixed link rule; add the subdir to run_tests.sh with nullglob so it runs them. - Bazel: build one cc_test per mysql test file via generate_unittests instead of globbing them all into a single brpc_mysql_test target, which duplicated main() and the FLAGS_mysql_* definitions (ld: duplicate symbol). Drop the per-file main()s so every suite relies on gtest_main, matching brpc_redis_unittest; tag the server-spawning suites external+local so bazel runs them unsandboxed with the real mysqld. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * address review comments: bind-sock flag, non-copyable response, naming - Controller: store the mysql-transaction BindSockAction in two bits of _flags (FLAGS_BIND_SOCK_RESERVE / FLAGS_BIND_SOCK_USE) via set_bind_sock_action()/bind_sock_action(), instead of a dedicated member, per review. No behavior change. - MysqlResponse: make it explicitly non-copyable (= delete copy ctor and assignment) and turn the previously no-op MergeFrom into a hard CHECK-failure, so an accidental copy/CopyFrom is caught instead of silently dropping parsed replies. - MysqlRequest: rename the trivial getters get_tx()/get_stmt() to tx()/stmt() per review. - ControllerPrivateAccessor: add set_mysql_statement_type() as a clearly named alias over the pipelined_count slot the mysql protocol reuses, and call it from the mysql protocol instead of set_pipelined_count() directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(mysql): move unittests from mysql/ subdir into test/ Move all seven mysql unittests (auth handshake/packet/scramble, connection_type, pool_concurrency, prepared/txn integration) and the test-plan doc out of test/mysql/ into test/, per review on #3310. The existing brpc_*_unittest glob in the Makefile and CMakeLists.txt now picks them up, so both revert to master with no mysql-specific lines. In BUILD.bazel they likewise join the brpc_unittests glob; the five tests that fork a real mysqld keep their ["external", "local"] tags by folding those entries into that target's per_test_tags instead of a separate mysql target. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(mysql): drop run_tests.sh mysql/ subdir entry after move The mysql unittests now live in test/ and match the existing brpc*unittest glob in run_tests.sh, so revert the script to master. The removed mysql/brpc*unittest entry needed `shopt -s nullglob` to skip a missing subdir, but CI runs the script under `sh` (dash), where shopt is absent; nullglob stayed off and the now-empty glob was executed as a literal path, exiting 127. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix backup/retry call hang: default-initialize bind_sock_action The new bind_sock_action member of Controller::Call had no in-class default, and the Call(Call*) copy constructor used for backup requests and retries did not initialize it in its member-init list. The backup call therefore read an indeterminate value in Controller::Call::OnComplete, which branches on it before the normal pool-return / SetFailed path; when the garbage matched BIND_SOCK_RESERVE/BIND_SOCK_USE the backup call's socket was diverted to _bind_sock or held instead of returned, leaving the in-flight RPC unconcluded and hanging ChannelTest.backup_request until the test timeout (nondeterministic, hence flaky-looking). Give the member an in-class default initializer (BIND_SOCK_NONE) so every Call construction path inherits it; a backup/retry never inherits transaction connection-affinity. This closes the whole class of "new init path forgets the member" rather than the single copy-ctor instance. * Initialize Call::bind_sock_action explicitly in every constructor Per review: drop the in-class default initializer for Call::bind_sock_action and set it explicitly in each Call constructor (the copy ctor used for backup/retry) and Call::Reset(). Leaving it uninitialized was the cause of the backup/retry-request hang; explicit per-constructor init keeps every path safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review: remove redundant comments and dead code, move _bind_sock.reset() to ResetNonPods() * Remove commented-out code in mysql_stmt example --------- Co-authored-by: rajvarun77 <287367605+rajvarun77@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bring up
redis-server(andmysql-server) for the Linux unittest jobs — both the make/CMake jobs and the Bazel jobs — via the sharedinstall-essential-dependenciesaction, so the Redis client integration tests actually run instead of being skipped when the server is absent.Redis. The Redis client tests fork a real
redis-serverfound onPATH. With the server installed they now run inclang-unittest(make) and — with theexternal+localtags onbrpc_redis_unittest— under the Bazel unittest jobs as well (gcc-unittest-with-bazel,clang-unittest-with-bazel,clang-unittest-bazel-with-babylon-and-new-pb), where they previously passed vacuously because the sandbox hid the binary. Under ASan the five fork-and-connect cases (sanity,keys_with_spaces,incr_and_decr,by_components,auth) are filtered out — they flake on Redis's slow startup versus a fixed connect wait — while the Redis codec/parser tests still run.MySQL.
mysql-serveris pre-provisioned for the in-progress MySQL protocol support (#2093); the MySQL client tests land in that follow-up. There are no MySQL tests in this PR yet — installing the server now avoids re-touching CI when they arrive.CI: the full Build and Test on Linux run is green (exercised on my fork).