Add ClickHouse mock harness for omclickhouse tests#125
Add ClickHouse mock harness for omclickhouse tests#125
Conversation
Summary of ChangesHello @rgerhards, 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 significantly enhances the testing infrastructure for the 'omclickhouse' module by introducing a lightweight, Python-based HTTP mock server. This mock server allows tests to verify the SQL statements generated by 'omclickhouse' without requiring a full ClickHouse instance, leading to more efficient and reliable continuous integration. Key tests have been converted to use this new harness, and the 'omclickhouse' module itself has been designated as 'core-supported' and 'mature'. Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 a mock server for ClickHouse tests, which is a great improvement for CI stability and speed. The implementation of the mock server and the test helpers is well done, and the conversion of existing tests to use this new mock infrastructure is a reasonable approach. My review includes a few suggestions to improve maintainability and code style, such as fixing indentation for consistency in tests/Makefile.am, refactoring repetitive test logic in tests/clickhouse-dflt-tpl.sh, and avoiding a global variable in the Python mock server for better encapsulation. Overall, this is a solid contribution that significantly improves the testing strategy for omclickhouse.
| clickhouse-basic.sh \ | ||
| clickhouse-mock.sh \ | ||
| clickhouse-mock-server.py \ | ||
| clickhouse-load.sh \ |
There was a problem hiding this comment.
The indentation for the newly added files in EXTRA_DIST uses spaces, while the rest of the file appears to use tabs. For consistency and to adhere to common Makefile conventions, please use tabs for indentation.
clickhouse-basic.sh \
clickhouse-mock.sh \
clickhouse-mock-server.py \
clickhouse-load.sh \
tests/clickhouse-dflt-tpl.sh
Outdated
| if ! printf '%s\n' "$ACTUAL" | grep -q "INSERT INTO rsyslog.SystemEvents"; then | ||
| echo "unexpected SQL payload: missing INSERT" | ||
| cat -n "$REQ_FILE" | ||
| error_exit 1 | ||
| fi | ||
|
|
||
| if ! printf '%s\n' "$ACTUAL" | grep -q "VALUES (7, 20,"; then | ||
| echo "unexpected SQL payload: severity/facility mismatch" | ||
| cat -n "$REQ_FILE" | ||
| error_exit 1 | ||
| fi | ||
|
|
||
| if ! printf '%s\n' "$ACTUAL" | grep -q "'tag'"; then | ||
| echo "unexpected SQL payload: tag missing" | ||
| cat -n "$REQ_FILE" | ||
| error_exit 1 | ||
| fi | ||
|
|
||
| if ! printf '%s\n' "$ACTUAL" | grep -q "' msgnum:00000000:'"; then | ||
| echo "unexpected SQL payload: message mismatch" | ||
| cat -n "$REQ_FILE" | ||
| error_exit 1 | ||
| fi |
There was a problem hiding this comment.
These consecutive grep checks on the payload are repetitive. To improve readability and maintainability, this logic could be refactored into a helper function.
For example, you could define a function after ACTUAL=$(cat "$REQ_FILE"):
check_payload() {
local pattern="$1"
local errmsg="$2"
if ! printf '%s\n' "$ACTUAL" | grep -q -- "$pattern"; then
echo "unexpected SQL payload: $errmsg"
cat -n "$REQ_FILE"
error_exit 1
fi
}Then, you can replace the series of if blocks with clearer, one-line calls:
check_payload "INSERT INTO rsyslog.SystemEvents" "missing INSERT"
check_payload "VALUES (7, 20," "severity/facility mismatch"
check_payload "'tag'" "tag missing"
check_payload "' msgnum:00000000:'" "message mismatch"This makes the test's intent much clearer at a glance.
| global STORE | ||
| STORE = RequestStore(sink_dir) |
There was a problem hiding this comment.
Using a global variable for STORE is generally discouraged as it can make code harder to reason about. A better pattern here is to make the RequestStore instance a class attribute of the handler.
Here's how you can refactor it:
-
Add a class attribute to
ClickHouseMockHandler:class ClickHouseMockHandler(BaseHTTPRequestHandler): """Minimal handler that understands ClickHouse-style POSTs.""" STORE: RequestStore ...
-
In
do_POST, access it viaself:# inside do_POST at line 55 ... else: self.STORE.store(payload)
-
Finally, in
main, initialize the class attribute instead of the global variable (replacing lines 85-86):ClickHouseMockHandler.STORE = RequestStore(sink_dir)
This change improves encapsulation and makes the relationship between the handler and the store explicit.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| clickhouse_mock_setup() { | ||
| if [ -z "$PYTHON" ]; then | ||
| echo "TESTBENCH ERROR: PYTHON variable is unset" | ||
| error_exit 100 | ||
| fi | ||
|
|
||
| export CLICKHOUSE_MOCK_BASE="${RSYSLOG_DYNNAME}.clickhouse_mock" | ||
| export CLICKHOUSE_MOCK_SINK_DIR="${CLICKHOUSE_MOCK_BASE}/requests" | ||
| export CLICKHOUSE_MOCK_READY_FILE="${CLICKHOUSE_MOCK_BASE}/ready" | ||
| mkdir -p "$CLICKHOUSE_MOCK_SINK_DIR" | ||
|
|
||
| export CLICKHOUSE_MOCK_PORT="$(get_free_port)" | ||
|
|
||
| "$PYTHON" "${srcdir}/clickhouse-mock-server.py" \ | ||
| --host 127.0.0.1 \ | ||
| --port "$CLICKHOUSE_MOCK_PORT" \ | ||
| --sink-dir "$CLICKHOUSE_MOCK_SINK_DIR" \ | ||
| --ready-file "$CLICKHOUSE_MOCK_READY_FILE" & |
There was a problem hiding this comment.
[P1] Use python3 when starting ClickHouse mock server
The new helper invokes the mock server with $PYTHON, but clickhouse-mock-server.py uses Python 3–only syntax (type annotations, http.server). The build system still calls AM_PATH_PYTHON without a version requirement, so on systems where configure picks a Python 2 interpreter the test harness will now fail with a SyntaxError before any omclickhouse tests run. Please run the server with python3 explicitly or require PYTHON to point to a Python 3 executable before calling it.
Useful? React with 👍 / 👎.
rgerhards
force-pushed
the
codex/review-omclickhouse-quality-and-testbench-coverage
branch
from
1656b43 to
cd8e139
Compare
- mark omclickhouse as core-supported in MODULE_METADATA\n- add clickhouse mock server and helper so basic coverage runs without ClickHouse\n- refactor a subset of clickhouse tests to use the mock, skip remaining heavy scripts\n\nAI-Agent: ChatGPT
- duplicate essential ClickHouse scenarios into new clickhouse-mock-* scripts that use the mock server helper\n- revert the legacy clickhouse test names to exit 77 so they clearly skip without a real ClickHouse\n- hook the mock variants into the Automake harness and distribution list alongside the existing scripts\n\nAI-Agent: Codex
Fixes the module metadata YAML to satisfy linting by removing inline backticks, wrapping long strings, and using folded blocks for notes. This resolves CI syntax errors introduced when documenting the mock ClickHouse coverage. AI-Agent: ChatGPT
rgerhards
force-pushed
the
codex/review-omclickhouse-quality-and-testbench-coverage
branch
from
cd8e139 to
91b2141
Compare
📖 Documentation Preview Ready!✅ Sphinx docs built and deployed for this PR. 🌐 Browse Online (Recommended):🔗 Quick Links:
🤖 This comment was generated by the docs workflow. |
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d8f660368083329ddc0158aeb3b8a8