Add SKIP_PROXYSQL for infrastructure-free test groups (unit tests)#5520
Add SKIP_PROXYSQL for infrastructure-free test groups (unit tests)#5520renecannao merged 3 commits intov3.0from
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds an infrastructure-free unit-test mode driven by Changes
Sequence DiagramsequenceDiagram
participant User
participant Ensure as "ensure-infras.bash"
participant Runner as "run-tests-isolated.bash"
participant Tester as "Test binaries (host)"
User->>Ensure: start test execution
Ensure->>Ensure: source group env.sh
alt SKIP_PROXYSQL == "1"
Ensure-->>User: log "Skipping ProxySQL/backends" and exit 0
else
Ensure->>Ensure: ensure ProxySQL & backends up
end
User->>Runner: invoke test run
Runner->>Runner: source group env.sh
alt SKIP_PROXYSQL == "1"
Runner->>Tester: discover & execute unit test binaries on host
Tester-->>Runner: return aggregated results
else
Runner->>Runner: verify Docker containers & infra
Runner->>Tester: run tests inside test container
Tester-->>Runner: return aggregated results
end
Runner-->>User: return aggregated exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a mechanism to skip the requirement for ProxySQL and backend infrastructure for specific test groups, primarily targeting unit tests. By setting the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Unit tests run standalone against libproxysql.a with stubs and need no ProxySQL instance or backend databases. This adds a SKIP_PROXYSQL flag that test groups can set in their env.sh to bypass all Docker infrastructure setup and run tests directly on the host. Changes: - ensure-infras.bash: source group env.sh early; exit immediately when SKIP_PROXYSQL=1 - run-tests-isolated.bash: source group env.sh early; skip ProxySQL and backend verification; run proxysql-tester.py directly on host instead of inside a Docker container - New test/tap/groups/unit-tests/env.sh with SKIP_PROXYSQL=1 - groups.json: add 18 unit test binaries to unit-tests-g1 group - Documentation: SKIP_PROXYSQL in env var tables and new section for infrastructure-free groups in test/infra/README.md
renecannao
force-pushed
the
unit-tests-skip-proxysql
branch
from
5519e54 to
4c4736c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SKIP_PROXYSQL support, which is a great feature for running unit tests without backend infrastructure. The implementation is solid and the documentation updates are clear and comprehensive. I've identified a couple of areas for improvement related to code duplication and structure in the bash scripts, which I've detailed in my comments. These changes would enhance the maintainability of the test infrastructure.
| if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" | ||
| elif [ -f "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" | ||
| fi |
There was a problem hiding this comment.
This block for sourcing the group's env.sh is duplicated in test/infra/control/run-tests-isolated.bash (lines 59-63). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this logic into a common script that can be sourced by both files.
For example, you could create test/infra/common/source-group-env.bash with the following content:
# This script is meant to be sourced.
# It sources the environment file for a given TAP_GROUP.
if [ -z "${TAP_GROUP}" ] || [ -z "${WORKSPACE}" ]; then
echo "ERROR: TAP_GROUP and WORKSPACE must be set before sourcing." >&2
return 1
fi
BASE_GROUP=$(echo "${TAP_GROUP}" | sed -E "s/[-_]g[0-9]+.*//")
if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" ]; then
source "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh"
elif [ -f "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" ]; then
source "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh"
fiThen, you could replace the duplicated block in both scripts with:
source "${SCRIPT_DIR}/../common/source-group-env.bash"| if [ "${SKIP_PROXYSQL}" = "1" ]; then | ||
| echo ">>> SKIP_PROXYSQL=1: Skipping ProxySQL and backend verification for group '${TAP_GROUP}'." | ||
| echo ">>> Running unit tests directly (no infrastructure needed)." | ||
| else | ||
| # VERIFICATION: Verify ProxySQL is running | ||
| PROXY_CONTAINER="proxysql.${INFRA_ID}" | ||
| echo ">>> Verifying ProxySQL container: ${PROXY_CONTAINER}" | ||
| if ! docker ps --format '{{.Names}}' | grep -q "^${PROXY_CONTAINER}$"; then | ||
| echo "ERROR: ProxySQL container ${PROXY_CONTAINER} is NOT running!" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # VERIFICATION: Verify all required backend containers are running | ||
| for INFRA_NAME in ${INFRAS_TO_CHECK}; do | ||
| echo ">>> Verifying Backend: ${INFRA_NAME}" | ||
| # Extract container names from the infra's docker-compose.yml | ||
| if [ -f "${WORKSPACE}/test/infra/${INFRA_NAME}/docker-compose.yml" ]; then | ||
| # Project name used by init script | ||
| COMPOSE_PROJECT="${INFRA_NAME}-${INFRA_ID}" | ||
| # Get all services for this project | ||
| RUNNING_CONTAINERS=$(docker ps --filter "label=com.docker.compose.project=${COMPOSE_PROJECT}" --format '{{.Names}}') | ||
| if [ -z "${RUNNING_CONTAINERS}" ]; then | ||
| if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst" ]; then LST_PATH="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst"; else LST_PATH="${WORKSPACE}/test/tap/groups/${BASE_GROUP}/infras.lst"; fi | ||
| echo "ERROR: Required infrastructure '${INFRA_NAME}' is NOT running." | ||
| if [ -f "${LST_PATH}" ]; then | ||
| echo "According to '${LST_PATH}', this infrastructure is mandatory for the '${TAP_GROUP}' group." | ||
| # VERIFICATION: Verify all required backend containers are running | ||
| for INFRA_NAME in ${INFRAS_TO_CHECK}; do | ||
| echo ">>> Verifying Backend: ${INFRA_NAME}" | ||
| # Extract container names from the infra's docker-compose.yml | ||
| if [ -f "${WORKSPACE}/test/infra/${INFRA_NAME}/docker-compose.yml" ]; then | ||
| # Project name used by init script | ||
| COMPOSE_PROJECT="${INFRA_NAME}-${INFRA_ID}" | ||
| # Get all services for this project | ||
| RUNNING_CONTAINERS=$(docker ps --filter "label=com.docker.compose.project=${COMPOSE_PROJECT}" --format '{{.Names}}') | ||
| if [ -z "${RUNNING_CONTAINERS}" ]; then | ||
| if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst" ]; then LST_PATH="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst"; else LST_PATH="${WORKSPACE}/test/tap/groups/${BASE_GROUP}/infras.lst"; fi | ||
| echo "ERROR: Required infrastructure '${INFRA_NAME}' is NOT running." | ||
| if [ -f "${LST_PATH}" ]; then | ||
| echo "According to '${LST_PATH}', this infrastructure is mandatory for the '${TAP_GROUP}' group." | ||
| fi | ||
| echo "Please run initialization for '${INFRA_NAME}' first (e.g. cd test/infra/${INFRA_NAME} && ./docker-compose-init.bash)." | ||
| exit 1 | ||
| fi | ||
| echo "Please run initialization for '${INFRA_NAME}' first (e.g. cd test/infra/${INFRA_NAME} && ./docker-compose-init.bash)." | ||
| echo "Found running containers: ${RUNNING_CONTAINERS//$'\n'/ }" | ||
| else | ||
| echo "ERROR: Infrastructure directory ${INFRA_NAME} not found!" | ||
| exit 1 | ||
| fi | ||
| echo "Found running containers: ${RUNNING_CONTAINERS//$'\n'/ }" | ||
| else | ||
| echo "ERROR: Infrastructure directory ${INFRA_NAME} not found!" | ||
| exit 1 | ||
| fi | ||
| done | ||
| done | ||
|
|
||
| echo ">>> INFRASTRUCTURE VERIFIED. LAUNCHING TEST RUNNER..." | ||
| fi | ||
|
|
||
| echo ">>> INFRASTRUCTURE VERIFIED. LAUNCHING TEST RUNNER..." | ||
| # SKIP_PROXYSQL path: run unit tests directly on the host, no Docker needed | ||
| if [ "${SKIP_PROXYSQL}" = "1" ]; then | ||
| echo ">>> Running unit tests directly (no Docker container)..." | ||
|
|
||
| # Execute the Python tester directly on the host | ||
| export WORKSPACE | ||
| export TAP_GROUP | ||
| python3 "${WORKSPACE}/test/scripts/bin/proxysql-tester.py" | ||
| exit $? | ||
| fi |
There was a problem hiding this comment.
The logic for handling SKIP_PROXYSQL=1 is currently split across two if statements. This can be simplified by using an 'early exit' pattern. This would make the script's control flow easier to follow.
You can place all the logic for the SKIP_PROXYSQL case at the beginning of this section and have it exit. The rest of the script would then represent the default execution path, without needing to be wrapped in an else block.
if [ "${SKIP_PROXYSQL}" = "1" ]; then
echo ">>> SKIP_PROXYSQL=1: Skipping ProxySQL and backend verification for group '${TAP_GROUP}'."
echo ">>> Running unit tests directly (no infrastructure needed)."
echo ">>> Running unit tests directly (no Docker container)..."
# Execute the Python tester directly on the host
export WORKSPACE
export TAP_GROUP
python3 "${WORKSPACE}/test/scripts/bin/proxysql-tester.py"
exit $?
fi
# VERIFICATION: Verify ProxySQL is running
PROXY_CONTAINER="proxysql.${INFRA_ID}"
echo ">>> Verifying ProxySQL container: ${PROXY_CONTAINER}"
if ! docker ps --format '{{.Names}}' | grep -q "^${PROXY_CONTAINER}$" ; then
echo "ERROR: ProxySQL container ${PROXY_CONTAINER} is NOT running!"
exit 1
fi
# VERIFICATION: Verify all required backend containers are running
for INFRA_NAME in ${INFRAS_TO_CHECK}; do
echo ">>> Verifying Backend: ${INFRA_NAME}"
# Extract container names from the infra's docker-compose.yml
if [ -f "${WORKSPACE}/test/infra/${INFRA_NAME}/docker-compose.yml" ]; then
# Project name used by init script
COMPOSE_PROJECT="${INFRA_NAME}-${INFRA_ID}"
# Get all services for this project
RUNNING_CONTAINERS=$(docker ps --filter "label=com.docker.compose.project=${COMPOSE_PROJECT}" --format '{{.Names}}')
if [ -z "${RUNNING_CONTAINERS}" ]; then
if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst" ]; then LST_PATH="${WORKSPACE}/test/tap/groups/${TAP_GROUP}/infras.lst"; else LST_PATH="${WORKSPACE}/test/tap/groups/${BASE_GROUP}/infras.lst"; fi
echo "ERROR: Required infrastructure '${INFRA_NAME}' is NOT running."
if [ -f "${LST_PATH}" ]; then
echo "According to '${LST_PATH}', this infrastructure is mandatory for the '${TAP_GROUP}' group."
fi
echo "Please run initialization for '${INFRA_NAME}' first (e.g. cd test/infra/${INFRA_NAME} && ./docker-compose-init.bash)."
exit 1
fi
echo "Found running containers: ${RUNNING_CONTAINERS//$'
'/ }"
else
echo "ERROR: Infrastructure directory ${INFRA_NAME} not found!"
exit 1
fi
done
echo ">>> INFRASTRUCTURE VERIFIED. LAUNCHING TEST RUNNER..."Documents groups.json format (one entry per line, compact arrays), group naming conventions, directory structure, and infrastructure-free groups.
There was a problem hiding this comment.
Pull request overview
Adds a new “infrastructure-free” execution mode intended to let the TAP orchestration run the unit-test group without starting ProxySQL/backends, by introducing SKIP_PROXYSQL=1 semantics in the infra control scripts and defining a new unit-tests group.
Changes:
- Add early
env.shsourcing +SKIP_PROXYSQL=1short-circuit behavior toensure-infras.bashandrun-tests-isolated.bash. - Introduce
test/tap/groups/unit-tests/env.shand register unit test binaries intest/tap/groups/groups.jsonunderunit-tests-g1. - Document
SKIP_PROXYSQLand the unit-tests group intest/README.mdandtest/infra/README.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/infra/control/ensure-infras.bash | Sources group env early and exits immediately when SKIP_PROXYSQL=1. |
| test/infra/control/run-tests-isolated.bash | Sources group env early; skips infra verification and attempts host-side test execution when SKIP_PROXYSQL=1. |
| test/tap/groups/unit-tests/env.sh | New group env enabling SKIP_PROXYSQL=1. |
| test/tap/groups/groups.json | Adds unit-tests-g1 membership for unit test binaries (plus existing unit strip-schema test). |
| test/infra/README.md | Documents SKIP_PROXYSQL and adds a section describing infrastructure-free groups. |
| test/README.md | Adds unit-tests-g1 and documents SKIP_PROXYSQL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Execute the Python tester directly on the host | ||
| export WORKSPACE | ||
| export TAP_GROUP | ||
| python3 "${WORKSPACE}/test/scripts/bin/proxysql-tester.py" | ||
| exit $? |
There was a problem hiding this comment.
In the SKIP_PROXYSQL=1 path, this script executes test/scripts/bin/proxysql-tester.py directly on the host, but that runner currently requires a ProxySQL admin connection and several environment variables that are normally set inside the Docker test-runner (e.g. TESTS_LOGS_PATH, TAP_ADMIN*). With SKIP_PROXYSQL=1 there is no ProxySQL to connect to, so proxysql-tester.py will fail immediately (it opens the admin connection unconditionally during initialization). Consider either (a) adding an explicit “unit-tests / no-proxysql” mode to proxysql-tester.py that skips admin-dependent setup/dumps and only runs test/tap/tests/unit/*-t, while also setting the minimal required env vars for logging, or (b) bypassing proxysql-tester.py here and directly execute the unit test binaries for unit-tests-g1 with TAP-style reporting.
| # Execute the Python tester directly on the host | |
| export WORKSPACE | |
| export TAP_GROUP | |
| python3 "${WORKSPACE}/test/scripts/bin/proxysql-tester.py" | |
| exit $? | |
| # Prepare logs path for unit tests when skipping ProxySQL | |
| TESTS_LOGS_PATH_HOST="${INFRA_LOGS_PATH:-${WORKSPACE}/ci_infra_logs}/${INFRA_ID}/tests" | |
| mkdir -p "${TESTS_LOGS_PATH_HOST}" | |
| chmod 777 "${TESTS_LOGS_PATH_HOST}" || true | |
| # Discover unit test binaries | |
| UNIT_TEST_DIR="${WORKSPACE}/test/tap/tests/unit" | |
| if [ ! -d "${UNIT_TEST_DIR}" ]; then | |
| echo "ERROR: Unit test directory not found: ${UNIT_TEST_DIR}" | |
| exit 1 | |
| fi | |
| # Collect all unit test binaries matching *-t | |
| shopt -s nullglob | |
| UNIT_TEST_BINARIES=( "${UNIT_TEST_DIR}"/*-t ) | |
| shopt -u nullglob | |
| if [ ${#UNIT_TEST_BINARIES[@]} -eq 0 ]; then | |
| echo "ERROR: No unit test binaries found in ${UNIT_TEST_DIR} (pattern '*-t')." | |
| exit 1 | |
| fi | |
| # Run unit tests with simple TAP-style reporting | |
| TOTAL_TESTS=${#UNIT_TEST_BINARIES[@]} | |
| echo "1..${TOTAL_TESTS}" | |
| EXIT_CODE=0 | |
| INDEX=0 | |
| for TEST_BIN in "${UNIT_TEST_BINARIES[@]}"; do | |
| INDEX=$((INDEX + 1)) | |
| TEST_NAME="$(basename "${TEST_BIN}")" | |
| if "${TEST_BIN}"; then | |
| echo "ok ${INDEX} - ${TEST_NAME}" | |
| else | |
| echo "not ok ${INDEX} - ${TEST_NAME}" | |
| EXIT_CODE=1 | |
| fi | |
| done | |
| exit ${EXIT_CODE} |
| # Source group env.sh to pick up SKIP_PROXYSQL and other group-level settings | ||
| if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" | ||
| elif [ -f "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" | ||
| fi |
There was a problem hiding this comment.
source ".../${TAP_GROUP}/env.sh" executes code from a path derived directly from TAP_GROUP/BASE_GROUP without validating that the group name is a simple, safe identifier (no /, .., whitespace, etc.). Previously these variables were used only for file reads, but sourcing turns this into code execution if someone can influence TAP_GROUP (e.g., in CI env). Suggest validating TAP_GROUP/BASE_GROUP against a strict allowlist regex (and rejecting/escaping anything else) before constructing paths and sourcing.
| # Source group env.sh to pick up SKIP_PROXYSQL and other group-level settings | ||
| if [ -f "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${TAP_GROUP}/env.sh" | ||
| elif [ -f "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" ]; then | ||
| source "${WORKSPACE}/test/tap/groups/${BASE_GROUP}/env.sh" | ||
| fi |
There was a problem hiding this comment.
Same concern as in ensure-infras.bash: this script sources env.sh from a path derived from TAP_GROUP/BASE_GROUP without validating the group name. Since source executes the file, please add a strict allowlist validation (e.g., only [A-Za-z0-9._=-] and no path separators) before sourcing, to avoid accidental or malicious code execution when TAP_GROUP is user/CI-provided.
| Some test groups do not require ProxySQL or any backend infrastructure. These groups set `SKIP_PROXYSQL=1` in their `env.sh`, which causes: | ||
|
|
||
| - `ensure-infras.bash` to exit immediately (no Docker containers started) | ||
| - `run-tests-isolated.bash` to run tests directly on the host (no Docker test runner) | ||
|
|
||
| The `unit-tests` group is the primary example. Unit tests link against `libproxysql.a` with stub globals and run as standalone binaries. |
There was a problem hiding this comment.
This section states that with SKIP_PROXYSQL=1, run-tests-isolated.bash will run tests directly on the host with “no infrastructure needed”. With the current implementation, the SKIP path still invokes proxysql-tester.py, which requires ProxySQL admin connectivity and Docker-provisioned env vars, so this documentation will be incorrect unless the runner is updated accordingly (or the SKIP path is changed to execute unit test binaries directly).
| # No ProxySQL or backend infrastructure required. | ||
| # Unit tests link against libproxysql.a with stubs and run standalone. | ||
|
|
||
| export SKIP_PROXYSQL=1 |
There was a problem hiding this comment.
This group’s unit test binaries live under test/tap/tests/unit/, but env.sh only sets SKIP_PROXYSQL=1 and doesn’t override TAP_WORKDIR/TAP_WORKDIRS to include that unit-test directory. With the current test runner (proxysql-tester.py), discovery is driven by TAP_WORKDIRS, so unit-tests-g1 will end up running 0 tests (it will ignore the regular TAP tests due to groups.json membership filtering, and won’t find the unit binaries). Recommend exporting TAP_WORKDIRS (and TAP_WORKDIR) to point at ${WORKSPACE}/test/tap/tests/unit/ for this group (or adjusting the SKIP runner to execute unit binaries directly without relying on TAP_WORKDIRS).
| export SKIP_PROXYSQL=1 | |
| export SKIP_PROXYSQL=1 | |
| # Point the TAP runner at the unit test binaries for this group. | |
| export TAP_WORKDIR="${WORKSPACE}/test/tap/tests/unit" | |
| export TAP_WORKDIRS="${TAP_WORKDIR}" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/infra/control/run-tests-isolated.bash`:
- Around line 140-148: The SKIP_PROXYSQL branch in run-tests-isolated.bash runs
proxysql-tester.py before the tester environment (e.g., TESTS_LOGS_PATH_HOST and
variables from env-isolated.bash) is created; update that branch to perform the
same bootstrap used later: source the existing env-isolated.bash (or call the
shared bootstrap helper if you extract one) and ensure TESTS_LOGS_PATH_HOST and
TAP_WORKDIR*/TAP_*PORT variables are exported before invoking python3
"${WORKSPACE}/test/scripts/bin/proxysql-tester.py" so the script (which reads
TESTS_LOGS_PATH, TAP_WORKDIR*, TAP_*PORT) always sees the expected environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb37a4ca-6326-4fc5-9790-a23684c76443
📒 Files selected for processing (6)
test/README.mdtest/infra/README.mdtest/infra/control/ensure-infras.bashtest/infra/control/run-tests-isolated.bashtest/tap/groups/groups.jsontest/tap/groups/unit-tests/env.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness
Applied to files:
test/tap/groups/unit-tests/env.shtest/README.mdtest/infra/control/run-tests-isolated.bashtest/infra/README.md
📚 Learning: 2026-01-20T09:34:27.165Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/groups/unit-tests/env.shtest/README.mdtest/infra/control/run-tests-isolated.bashtest/infra/README.md
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/groups/unit-tests/env.sh
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags
Applied to files:
test/tap/groups/unit-tests/env.sh
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Feature tiers are controlled by build flags: PROXYSQL31=1 for v3.1.x, PROXYSQLGENAI=1 for v4.0.x; PROXYSQLGENAI=1 implies PROXYSQL31=1
Applied to files:
test/tap/groups/unit-tests/env.shtest/README.md
📚 Learning: 2026-03-22T14:38:16.082Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.082Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp
Applied to files:
test/README.mdtest/infra/README.md
🪛 Shellcheck (0.11.0)
test/tap/groups/unit-tests/env.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
test/infra/control/ensure-infras.bash
[warning] 32-32: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 34-34: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
test/infra/control/run-tests-isolated.bash
[warning] 60-60: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
[warning] 62-62: ShellCheck can't follow non-constant source. Use a directive to specify location.
(SC1090)
🔇 Additional comments (6)
test/README.md (1)
39-39: Top-level docs expose the new mode clearly.Adding
unit-tests-g1to the table and documentingSKIP_PROXYSQLhere makes the infrastructure-free flow easy to discover from the main test README.Also applies to: 53-53
test/tap/groups/unit-tests/env.sh (1)
1-5: Nice minimal base-group env toggle.Keeping this as a tiny
unit-tests/env.shfile lines up with theBASE_GROUPfallback in the control scripts and avoids hard-coding special cases elsewhere.test/infra/control/ensure-infras.bash (1)
30-42: Early skip path is in the right place.Sourcing group env before infra resolution and exiting before any Docker or hook work keeps infrastructure-free groups from touching containers at all.
test/infra/README.md (1)
119-119: The infrastructure-free section reads clearly.The env-var reference plus Section 6.1 make the
unit-tests-g1flow easy to follow and clearly explain why these groups do not need aninfras.lst.Also applies to: 178-209
test/infra/control/run-tests-isolated.bash (2)
58-63: Base-group env fallback looks good.Loading
${TAP_GROUP}/env.shfirst and then${BASE_GROUP}/env.shkeepsunit-tests-g1aligned withtest/tap/groups/unit-tests/env.shwithout adding a one-off path.
100-138: Good: infra-backed groups keep the existing guardrails.Keeping the ProxySQL/backend verification under the non-skip branch limits the behavior change to
SKIP_PROXYSQL=1groups.
…l-tester.py proxysql-tester.py requires a ProxySQL admin connection and Docker-specific env vars (TESTS_LOGS_PATH, TAP_ADMINPORT, TAP_WORKDIR, etc.) that don't exist when SKIP_PROXYSQL=1. Instead, discover test binaries from groups.json and execute them directly on the host with a simple pass/fail summary. Addresses review feedback from CodeRabbit, Copilot, and Gemini on PR #5520.
|
❌ The last analysis has failed. |
Summary
SKIP_PROXYSQL=1support so test groups can run without ProxySQL or any backend infrastructureunit-testsgroup with all 18 unit test binaries from Unit test infrastructure: Glo* stubs, init helpers, and smoke test #5482 plus the existingunit-strip_schema_from_query-ttest/README.mdandtest/infra/README.mdHow it works
When a group's
env.shexportsSKIP_PROXYSQL=1:ensure-infras.bash: sources the group'senv.shearly and exits immediately — no ProxySQL container, no backend containers, no hooksrun-tests-isolated.bash: sources the group'senv.shearly, skips all ProxySQL/backend verification, and runsproxysql-tester.pydirectly on the host instead of inside a Docker containerFiles changed
test/infra/control/ensure-infras.bashenv.shearly; early-exit onSKIP_PROXYSQL=1test/infra/control/run-tests-isolated.bashenv.shearly; skip verification and Docker whenSKIP_PROXYSQL=1test/tap/groups/unit-tests/env.shSKIP_PROXYSQL=1test/tap/groups/groups.jsonunit-tests-g1test/README.mdunit-tests-g1to group table;SKIP_PROXYSQLto env varstest/infra/README.mdSKIP_PROXYSQLto env vars; new section 6.1 for infrastructure-free groupsTest plan
unit-tests-g1runs successfully via multi-group runnerlegacy-g1) are unaffected by the env.sh sourcing changeensure-infras.bashexits cleanly withSKIP_PROXYSQL=1Summary by CodeRabbit
Documentation
New Features