Add a CLI regression test target - #330
Conversation
![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.
Reviewed commit: 3c8b06a379
ℹ️ 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 "@codex address that feedback".
| ALL_ALGOS="2q 3LCache CAR GLCache RandomLRU arc arcv0 cacheus clock clock2qplus | ||
| clockpro fifo fifo-merge fifo-reinsertion fifomerge flashProb gdsf gl-cache | ||
| lecar lecarv0 lfu lfucpp lfuda lhd lirs lrb lru lru-k lru-prob nop | ||
| pluginCache qdlp random randomTwo s3-fifo s3-fifov0 s3fifo s3fifod s3fifov0 | ||
| sieve size slru slruv0 tinyLFU twoq wtinyLFU" |
There was a problem hiding this comment.
ALL_ALGOS omits hyperbolic, belady, and beladySize, although all three are registered in libCacheSim/cache/cacheAlgoRegistry.c and the selected oracle trace makes the Belady policies valid. Consequently, neither the -e print sweep nor the replay sweep exercises their cachesim initialization and teardown paths, so regressions in these built-in algorithms can pass this target despite the stated all-algorithm coverage; the later Belady MINISIM checks exercise a different executable path and do not close this gap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new CLI-focused regression test to the test/ suite to exercise end-to-end argument parsing and “run to completion” behavior that isn’t covered by in-process C unit tests. This fits into the codebase’s CTest-driven testing by introducing a testCLI target that runs a shell script from the build directory.
Changes:
- Add
test/test_cli.sh, a bash-based regression suite that drivescachesim,traceAnalyzer, andmrcProfileracross many option/parameter combinations. - Wire the script into CTest as a new
testCLItest viatest/CMakeLists.txt.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_cli.sh | New bash regression suite that runs CLI binaries and asserts clean failures vs crashes across many parameter/option cases. |
| test/CMakeLists.txt | Registers the new testCLI CTest target to run the shell regression suite from the build directory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ALL_ALGOS="2q 3LCache CAR GLCache RandomLRU arc arcv0 cacheus clock clock2qplus | ||
| clockpro fifo fifo-merge fifo-reinsertion fifomerge flashProb gdsf gl-cache | ||
| lecar lecarv0 lfu lfucpp lfuda lhd lirs lrb lru lru-k lru-prob nop | ||
| pluginCache qdlp random randomTwo s3-fifo s3-fifov0 s3fifo s3fifod s3fifov0 | ||
| sieve size slru slruv0 tinyLFU twoq wtinyLFU" |
| add_test(NAME testCLI | ||
| COMMAND ${CMAKE_COMMAND} -E env bash ${CMAKE_CURRENT_SOURCE_DIR}/test_cli.sh | ||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) |
| TRACE_CSV="${DATA_DIR}/cloudPhysicsIO.csv" | ||
| TRACE_TXT="${DATA_DIR}/cloudPhysicsIO.txt" | ||
|
|
||
| WORK_DIR=$(mktemp -d) |
720f1d5 to
aa99429
Compare
3c8b06a to
803fa29
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 803fa298de
ℹ️ 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 "@codex address that feedback".
| elif ! grep -qi "error" <<<"${out}"; then | ||
| _report 1 "${desc} failed without an error message (exit ${rc})" | ||
| else | ||
| _report 0 "" |
There was a problem hiding this comment.
When this suite is run against an AddressSanitizer build, a segfault is typically converted into exit code 1 with output containing ERROR: AddressSanitizer; this branch therefore counts the crash as a clean rejection solely because the output contains “error.” That allows the invalid-input crash regressions this helper is intended to detect to pass under a common diagnostic configuration, so sanitizer error signatures should be checked before the generic message check.
Useful? React with 👍 / 👎.
| # The script prints SKIP: and exits 0 when the binaries or sample traces are | ||
| # absent. Without this, ctest reports that as a pass, so a run that tested | ||
| # nothing looks the same as a run that tested everything. | ||
| set_tests_properties(testCLI PROPERTIES SKIP_REGULAR_EXPRESSION "^SKIP:") |
There was a problem hiding this comment.
On CMake 3.12–3.15, which the root cmake_minimum_required(VERSION 3.12) explicitly supports, SKIP_REGULAR_EXPRESSION is unavailable because CMake introduced this test property in 3.16. Consequently, a missing-binary or missing-data run exits zero and is reported as passed rather than skipped on those supported versions; either raise the minimum to 3.16 or use a skip mechanism available in 3.12.
Useful? React with 👍 / 👎.
aa99429 to
cddf41e
Compare
803fa29 to
dbe53fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbe53fcc0d
ℹ️ 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 "@codex address that feedback".
|
|
||
| n_skipped=0 | ||
| for algo in ${ALL_ALGOS}; do | ||
| out=$("${BIN_DIR}/cachesim" "${TRACE_ORACLE}" oracleGeneral "${algo}" 1gb -e print 2>&1) |
There was a problem hiding this comment.
-e print
When built with ENABLE_LRB or ENABLE_3L_CACHE, this sweep includes the now-registered lrb and 3LCache entries, but their parsers reject bare print before reaching their print branches because value is NULL (LRB_Interface.cpp:333-338 and ThreeLCache_Interface.cpp:343-348). These configurations therefore record a failure here on every ctest run; either fix those parsers as part of this change or exclude/handle algorithms that cannot accept -e print.
Useful? React with 👍 / 👎.
| "${BIN_DIR}/traceAnalyzer" --num-req=10000 -o out-nreq "${TRACE}" vscsi | ||
| fi | ||
|
|
||
| if [[ -x "${BIN_DIR}/mrcProfiler" ]]; then |
There was a problem hiding this comment.
If mrcProfiler is absent or moved from bin/—for example after building only the cachesim target—this condition silently skips all SHARDS and MINISIM checks, yet the script can still exit successfully and CTest reports testCLI as passed. mrcProfiler and traceAnalyzer are unconditional project executables rather than optional features, so the preflight should require them (or skip/fail the whole test) instead of treating their missing coverage as success.
Useful? React with 👍 / 👎.
cddf41e to
74d0d5e
Compare
dbe53fc to
f3a18a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3a18a6d68
ℹ️ 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 "@codex address that feedback".
| local out | ||
| out=$("$@" 2>&1) | ||
| local rc=$? | ||
| if [[ ${rc} -eq 0 ]]; then |
There was a problem hiding this comment.
When this target runs against a recovering sanitizer such as UBSan, a command can print runtime error: and still exit 0; this branch therefore records the invocation as passing. The same exit-code-only behavior appears in expect_output and the manual algorithm sweeps, so undefined behavior in most valid-input paths can evade the regression target even though expect_clean_error already recognizes these diagnostics. Check sanitizer signatures before accepting successful commands as well.
Useful? React with 👍 / 👎.
74d0d5e to
7393ce8
Compare
f3a18a6 to
70efb28
Compare
|
Consolidated response to the review comments on this PR. Sanitizer diagnostics counted as clean rejections — fixed, and it was the best catch here. The follow-up — that a successful command can also print
Missing algorithms — fixed.
Not addressed, both real: Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70efb28ccb
ℹ️ 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 "@codex address that feedback".
| if [[ ${rc} -ne 0 ]] && grep -qi "do not support algorithm" <<<"${out}"; then | ||
| n_skipped=$((n_skipped + 1)) | ||
| continue |
There was a problem hiding this comment.
If an always-built entry such as lru is accidentally removed from g_cache_algos, create_cache_by_name() returns NULL and cache_init.h emits this exact “do not support algorithm” message, so this branch skips the failure; the replay sweep repeats the same logic. Only 3LCache, GLCache/gl-cache, and lrb are feature-gated, so restrict skipping to those optional names or pass the enabled feature set into the test, otherwise the target can pass after a mandatory CLI algorithm disappears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and fixed — this one undermined the sweep's whole purpose, so it was worth taking even though I had otherwise stopped adding to this PR.
The skip keyed off the do not support algorithm message alone, but cache_init.h emits that whenever a name is missing from the registry for any reason, not just an uncompiled feature. So a mandatory algorithm dropping out of g_cache_algos — precisely the regression the registry refactor could introduce — would have been skipped rather than reported.
Your list is exactly right: 3LCache, GLCache, gl-cache and lrb are the only feature-gated entries. The skip is restricted to those now.
Verified by deleting an entry and rebuilding, using gdsf rather than lru — lru is asserted directly elsewhere in the suite, so its removal would have been caught regardless, whereas gdsf is covered by the sweeps and nothing else, which is the case that was genuinely silent:
FAIL: gdsf -e print (exit 134) do not support algorithm gdsf
FAIL: gdsf replay (exit 134) do not support algorithm gdsf
(4 algorithms not compiled in, skipped)
The four genuinely optional ones still skip, and the suite is 180/180 with the registry restored.
Generated by Claude Code
70efb28 to
482d28d
Compare
7393ce8 to
7d8f16e
Compare
482d28d to
8c87ad4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c87ad4142
ℹ️ 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 "@codex address that feedback".
| --size=100MB,500MB,3 2>/dev/null | grep '^104857600B' | awk '{printf "%.4f", $2}') | ||
| _cachesim_exact=$("${BIN_DIR}/cachesim" "${TRACE_ORACLE}" oracleGeneral lru 100mb \ | ||
| 2>/dev/null | tail -1 | grep -oE 'miss ratio [0-9.]+' | head -1 | awk '{printf "%.4f", $3}') | ||
| if [[ "${_minisim_unsampled}" == "${_cachesim_exact}" ]]; then |
There was a problem hiding this comment.
If both extraction pipelines fail to find their expected rows—for example after both tools' output formats change—each command substitution produces an empty string, and this equality check records the regression assertion as passed because "" == ""; the pipeline statuses are discarded because the script does not use set -e. Require both extracted values to be nonempty before comparing them so this test cannot succeed without actually observing either miss ratio.
Useful? React with 👍 / 👎.
7d8f16e to
52d2e27
Compare
8c87ad4 to
73893f3
Compare
Part 5 of 5, split out of #324. Stacked on #329 — the diff shown here is against that branch; merge #328 and #329 first.
Two files:
test/test_cli.shand itstest/CMakeLists.txtwiring, as a newtestCLIctest target. 174 checks over ground nothing covered before.Why a shell target rather than more C unit tests
The existing C tests build caches in-process. Every bug fixed in #328 and #329 lives in the path between the command line and that point — argp callbacks, parameter parsing, the
-e printearly exit — or only becomes visible when a binary runs to completion. So this drives the actual binaries.Two design choices carried most of the value:
s3fifov0andflashProbcrashes were found; both were absent from every list I would have written by hand.-e printcallsexit(0)before teardown, and LeakSanitizer still counts that memory as reachable, so a parse-only test sees nothing.Invalid input is asserted to fail cleanly — non-zero with a message, but not SIGSEGV, SIGFPE or SIGBUS. The project's
ERROR()aborts, so a deliberate rejection has to stay distinguishable from a crash, and several of these bugs presented as exactly that difference.Coverage
-o,--verbose,-e printfor every algorithmSLRUn-segat 0/-1/4/16/20 andseg-sizeempty /0:0/ 24 entriesFIX_SIZE— rate 1 is the undefined-behavior case, whereUINT64_MAX * sample_raterounds to 2^64 and overflows the castcachesimon unsampled runs, not merely exiting 0, so the size-scaling bug cannot come back quietlybeladyandbeladySizeaccepted on oracle traces and rejected on others--hashpowerat 4, 5, 6 and 8 across the composite policieswtinyLFU --consider-obj-metadata=true, which used to segfaultobj_id = 0fails the test rather than returning a believable numberAre the tests load-bearing?
Green tests prove nothing on their own, so I checked the other direction: reverting the six originally-fixed sources to their previous state and rebuilding gives 16 failures, and 0 with them restored.
Testing
ctest --output-on-failure— 10/10, both plain Release and under-fsanitize=leakwith CI'sASAN_OPTIONS.Generated by Claude Code