Expanded automated test coverage across the E2E journey suite - #61
Conversation
…089/092/096/098/099/105/107/108/109/110/113/114/115/116/138 in journey.sh Coverage: - TC-043: iceberg_metadata() Parquet file probe - TC-058: pg_dump excludes S3/GCS/Azure credentials - TC-059: Lakekeeper rejects bad-credential warehouse request - TC-062: cold rows survive SeaweedFS container restart - TC-063: PG extensions and DuckDB secret reload after pg restart - TC-066/067/070: partition column types (timestamp no-tz, date, text rejected) - TC-087/089/092/096/098/099/105: extended type round-trips (timestamp, time, char, json, interval, oid, full-column-set) - TC-107/108/109: multi-table isolation (failure, list config, disable-one) - TC-110: idempotent re-register updates config, no duplicate row - TC-113/114: unsupported table types (UNLOGGED, non-existent) rejected at register - TC-115/116: LIST and HASH partition strategies rejected at archive time - TC-138: same table name in two schemas yields distinct Iceberg namespaces
…gged register behavior, partition PK inheritance, and bound-parse grep patterns
…t cascade failures
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesTiered journey coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
docs/animated_diagram.html (4)
752-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
currentStepIndexis dead state.It's declared here and assigned in
runAction(Line 903) but never read. Drop it, or wire it into the flows if step tracking was intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/animated_diagram.html` around lines 752 - 753, Remove the unused currentStepIndex state declaration and its assignment in runAction, unless step tracking is intentionally required; do not alter the surrounding activeTimeline flow.
868-872: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLabel the play/pause button for assistive tech.
The button's only content is a glyph, so it has no meaningful accessible name and its state isn't exposed.
♿ Proposed fix
function togglePlay() { isPlaying = !isPlaying; - document.getElementById('play-pause').innerText = isPlaying ? "⏸" : "▶"; + const btn = document.getElementById('play-pause'); + btn.innerText = isPlaying ? "⏸" : "▶"; + btn.setAttribute('aria-label', isPlaying ? 'Pause simulation' : 'Play simulation'); logToConsole(isPlaying ? "Simulation resumed." : "Simulation paused."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/animated_diagram.html` around lines 868 - 872, Update togglePlay so the play-pause button exposes a meaningful accessible label that reflects its current state, such as “Pause” while playing and “Play” while paused, alongside the existing glyph and console logging behavior.
553-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the speed slider an accessible name.
The "Slower"/"Faster" spans aren't programmatically associated, so the range input is announced without a label.
♿ Proposed fix
- <input type="range" class="speed-slider" id="speed-range" min="1" max="10" value="5" oninput="updateSpeed()"> + <input type="range" class="speed-slider" id="speed-range" min="1" max="10" value="5" + aria-label="Animation speed" oninput="updateSpeed()">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/animated_diagram.html` around lines 553 - 560, Give the range input in the animation-controls block an accessible programmatic name by associating it with a visible label or adding an appropriate accessible naming attribute. Preserve the existing slider behavior and “Slower”/“Faster” visual guidance.
633-646: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTooltips are mouse-only.
showTooltip/hideTooltipare bound toonmouseover/onmouseouton non-focusable<g>elements, so keyboard and screen-reader users can never reach the component descriptions. Consider addingtabindex="0"plusonfocus/onblurhandlers on each node group (and positioning offgetBoundingClientRect()of the node when the event has no cursor coordinates).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/animated_diagram.html` around lines 633 - 646, The node groups node-client and node-hook are only tooltip-accessible by mouse. Make each group keyboard-focusable with tabindex="0", add matching onfocus and onblur handlers that invoke showTooltip and hideTooltip, and ensure tooltip positioning falls back to the focused node’s getBoundingClientRect() when no cursor coordinates are available; apply the same accessibility behavior to every node group in the diagram.ci/journey.sh (4)
3180-3189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTop comment and step() message contradict each other on the expected rejection point.
The block header (line 3181) states the story is about "UNLOGGED partitioned table rejected at register," but the
step()message inside the function (line 3186) says "register accepted; fails at archive time." The implementation itself is intentionally lenient about which stage rejects (usingnotefor the unexpected path), so the header title is misleading about what's actually being asserted.📝 Align header with actual behavior
-# Story — TC-113: UNLOGGED partitioned table rejected at register. +# Story — TC-113: UNLOGGED partitioned table — rejected at register or archive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/journey.sh` around lines 3180 - 3189, Align the TC-113 story header comment with the behavior described by step() in story_unlogged_rejected: state that registration is accepted and rejection occurs at archive time. Do not change the implementation or its intentionally lenient stage handling.
2719-2743: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
bash -cwith interpolated${SW_IP}re-parses a variable as shell code.Static analysis flags this as CWE-78 (OS command injection pattern):
bash -c ">/dev/tcp/${SW_IP}/8333"re-parses the interpolated string as a new shell command. In practiceSW_IPlooks like it's derived from Docker network inspection (not attacker input), so exploitability here is minimal, but the pattern is worth avoiding for defensive hygiene — a subshell redirection avoids the re-parse entirely:🛡️ Safer TCP-check pattern
- until bash -c ">/dev/tcp/${SW_IP}/8333" 2>/dev/null; do + until (exec 3<>"/dev/tcp/${SW_IP}/8333") 2>/dev/null; do🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/journey.sh` around lines 2719 - 2743, Replace the interpolated bash -c TCP probe in story_seaweedfs_restart with a direct subshell redirection to /dev/tcp using SW_IP, avoiding re-parsing the address as shell code. Preserve the existing retry counter, timeout, failure message, and post-readiness delay.Source: Linters/SAST tools
3316-3334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnquoted heredoc requires manual
\$escaping, unlike every sibling story.This heredoc uses
<<EOSQL(unquoted), forcing the PL/pgSQL dollar-quote delimiters to be manually escaped (\$do\$at lines 3323 and 3331) to avoid bash treating$do$as variable expansion. Every other SQL heredoc added in this PR (TC-066, TC-067, TC-107, TC-109) uses the safer<<'EOSQL'(quoted) form, which needs no escaping since no shell variables are actually referenced in this block. Using the quoted form here would be more consistent and remove the risk of a missed backslash silently breaking the SQL or (in the worst case) triggering unintended expansion if a$-prefixed name is ever added later.♻️ Use the same quoted-heredoc convention as sibling stories
- qf "$HOST" <<EOSQL >/dev/null + qf "$HOST" <<'EOSQL' >/dev/null CREATE TABLE IF NOT EXISTS public.sa_items ( ... -DO \$do\$ +DO $do$ DECLARE m date; BEGIN m := (date_trunc('month', now()) - interval '2 months')::date; EXECUTE format('CREATE TABLE IF NOT EXISTS public.%I PARTITION OF public.sa_items FOR VALUES FROM (%L) TO (%L)', 'sa_items_p_' || to_char(m, 'YYYY_MM'), m, (m + interval '1 month')); EXECUTE format('CREATE TABLE IF NOT EXISTS sc2.%I PARTITION OF sc2.sa_items FOR VALUES FROM (%L) TO (%L)', 'sa_items_p_' || to_char(m, 'YYYY_MM'), m, (m + interval '1 month')); -END \$do\$; +END $do$;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/journey.sh` around lines 3316 - 3334, Update the SQL heredoc in the qf block to use a quoted EOSQL delimiter, matching the sibling story blocks. Remove the manual backslash escapes from the PL/pgSQL dollar-quote delimiters so the SQL remains unchanged while avoiding shell expansion.
2775-2821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ret_dayscomputation is duplicated verbatim across 6 new stories.
local ret_days=$(( ( $(date -u +%s) - $(date -u -d "$(date -u +%Y-%m-01) -1 month" +%s) ) / 86400 ))is repeated identically here and at lines 2849, 2951, 3058, 3121, and 3315. Consider extracting a shared helper (e.g.retention_days_since_last_month()inci/lib.sh) so the date-arithmetic logic has one source of truth.♻️ Suggested helper
+# ci/lib.sh +retention_days_since_last_month() { + echo $(( ( $(date -u +%s) - $(date -u -d "$(date -u +%Y-%m-01) -1 month" +%s) ) / 86400 )) +}- local ret_days=$(( ( $(date -u +%s) - $(date -u -d "$(date -u +%Y-%m-01) -1 month" +%s) ) / 86400 )) + local ret_days; ret_days=$(retention_days_since_last_month)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/journey.sh` around lines 2775 - 2821, Extract the duplicated ret_days date arithmetic into a shared retention_days_since_last_month helper in ci/lib.sh, then replace the local calculations in story_partition_col_timestamp and the other five stories with calls to that helper. Preserve the current UTC-based result and days-since-start-of-last-month behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/journey.sh`:
- Around line 2922-2932: Update the typed_ext table definition’s col_oid column
from bigint to PostgreSQL oid, and remove the explicit bigint casts in both
INSERT statements so values are inserted as oid. Keep the TC-099 assertions and
surrounding typed-column coverage unchanged, ensuring the archiver/export path
exercises oid-to-bigint mapping.
- Around line 3345-3358: Update the TC-138 cleanup sequence to drop
public.sa_items as a view before dropping the underlying table, following the
existing cleanup pattern. Add the view drop before the existing DROP TABLE
statement, preserving the current cleanup order for coldfront metadata and sc2
schema removal.
In `@docs/animated_diagram.html`:
- Around line 912-944: Add a runToken generation counter near the animation
state and increment it in resetAnimationState before clearing visual state;
update animatePacket and delayStep to capture the current token and abort every
requestAnimationFrame, timeout, and continuation when it no longer matches.
Ensure self-restarting flows such as runTieredSelect use resetAnimationState to
invalidate prior chains, including concurrent fan-out animatePacket calls.
- Around line 1292-1298: Update resetAnimationState to restore watermark-text to
its initial value along with the existing partition labels and colors. Ensure
each animation loop starts with the original watermark and Phase 1 state, rather
than retaining the Phase 5 value of 2026-04-01.
- Around line 964-973: Update the animation timing logic in step so pausing
preserves accumulated packet progress and does not reset start or continuously
re-schedule requestAnimationFrame. Track the pause duration and shift start
forward when playback resumes, allowing progress to continue from its prior
position. Use an explicit start === null check so a timestamp of 0 remains
valid.
- Around line 829-830: Update the archive-button state handling around archBtn
to set its disabled property instead of relying on pointerEvents = 'none',
including the corresponding tiered-branch handling. Preserve the visual opacity
change while ensuring keyboard activation is blocked whenever the button is
unavailable.
---
Nitpick comments:
In `@ci/journey.sh`:
- Around line 3180-3189: Align the TC-113 story header comment with the behavior
described by step() in story_unlogged_rejected: state that registration is
accepted and rejection occurs at archive time. Do not change the implementation
or its intentionally lenient stage handling.
- Around line 2719-2743: Replace the interpolated bash -c TCP probe in
story_seaweedfs_restart with a direct subshell redirection to /dev/tcp using
SW_IP, avoiding re-parsing the address as shell code. Preserve the existing
retry counter, timeout, failure message, and post-readiness delay.
- Around line 3316-3334: Update the SQL heredoc in the qf block to use a quoted
EOSQL delimiter, matching the sibling story blocks. Remove the manual backslash
escapes from the PL/pgSQL dollar-quote delimiters so the SQL remains unchanged
while avoiding shell expansion.
- Around line 2775-2821: Extract the duplicated ret_days date arithmetic into a
shared retention_days_since_last_month helper in ci/lib.sh, then replace the
local calculations in story_partition_col_timestamp and the other five stories
with calls to that helper. Preserve the current UTC-based result and
days-since-start-of-last-month behavior.
In `@docs/animated_diagram.html`:
- Around line 752-753: Remove the unused currentStepIndex state declaration and
its assignment in runAction, unless step tracking is intentionally required; do
not alter the surrounding activeTimeline flow.
- Around line 868-872: Update togglePlay so the play-pause button exposes a
meaningful accessible label that reflects its current state, such as “Pause”
while playing and “Play” while paused, alongside the existing glyph and console
logging behavior.
- Around line 553-560: Give the range input in the animation-controls block an
accessible programmatic name by associating it with a visible label or adding an
appropriate accessible naming attribute. Preserve the existing slider behavior
and “Slower”/“Faster” visual guidance.
- Around line 633-646: The node groups node-client and node-hook are only
tooltip-accessible by mouse. Make each group keyboard-focusable with
tabindex="0", add matching onfocus and onblur handlers that invoke showTooltip
and hideTooltip, and ensure tooltip positioning falls back to the focused node’s
getBoundingClientRect() when no cursor coordinates are available; apply the same
accessibility behavior to every node group in the diagram.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0417b260-e081-44fb-8832-28ab1241522e
📒 Files selected for processing (2)
ci/journey.shdocs/animated_diagram.html
|
@coderabbitai I suspect the following may be true:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== journey script outline ==='
ast-grep outline ci/journey.sh --items all 2>/dev/null || true
printf '%s\n' '=== requested test-case locations and shared config references ==='
rg -n -C 5 \
-e 'TC-(043|058|059|062|070|099|107|109|113|114|115|116|138)' \
-e '/tmp/journey-archiver\.yaml' \
-e 'ret_days=\$\(\(' \
ci/journey.sh || true
printf '%s\n' '=== compose files and SeaweedFS volume configuration ==='
fd -a -t f 'docker-compose.matrix.yml|compose.*matrix.*\.ya?ml|.*matrix.*\.ya?ml' . -x sh -c '
echo "--- $1"
rg -n -C 8 "seaweed|volumes:|/data" "$1" || true
' sh {} \;
printf '%s\n' '=== archiver logging around table-name output ==='
rg -n -i -C 4 \
-e 'log\.(Info|Debug|Error|Warn)|slog\.(Info|Debug|Error|Warn)|logger\.' \
-e 'table.*name|table_name|TableName' \
-g '*.go' . || trueLength of output: 115354 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== TC-107 through TC-116 implementations ==='
sed -n '3028,3302p' ci/journey.sh
printf '%s\n' '=== TC-138 implementation ==='
sed -n '3305,3360p' ci/journey.sh
printf '%s\n' '=== archiver registration loading and run loop ==='
rg -n -C 12 -e 'LoadTables\(' -e 'runCycle\(' -e 'for .*range .*tables' cmd/archiver
printf '%s\n' '=== partitioner/archiver command ownership and hot period validation ==='
rg -n -C 8 -e 'os\.Args|os\.Executable|Owner|PartitionOnly|Tiered|hot-period' \
cmd/partitioner cmd/archiver internal/partcfg 2>/dev/null || true
printf '%s\n' '=== PostgreSQL oid mapping and rejection coverage ==='
rg -n -C 6 -e '"oid"|oid\b|pgFormatTypeToDuckDB' cmd/archiver ci/journey.shLength of output: 94216
The most consequential fixes are TC-058, TC-107, TC-114, TC-138, and registry isolation for the negative archive tests. |
|
Fixing some of the tests which were passing without exercising their premise:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci/journey.sh (1)
2942-2942: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRejection greps run against a log that already contains
registeroutput.In all three stories the register step writes its output to the same file (
>), andarchive_onlyappends the archive run (>>). The subsequentgrep -qi "…unsupported\|LIST\|HASH\|parse.*bound…"therefore can be satisfied by text produced before the archive ran, so the "rejected at archive time" assertion isn't strictly attributable to the archiver run. Using a separate log for the archive pass makes the attribution exact.♻️ Example for TC-070
- if archive_only "schema_name='public' AND table_name='tc070_textpart'" /tmp/journey-archiver.yaml /tmp/journey-textpart.log; then + : > /tmp/journey-textpart-arch.log + if archive_only "schema_name='public' AND table_name='tc070_textpart'" /tmp/journey-archiver.yaml /tmp/journey-textpart-arch.log; then fail "TC-070: archiver should have rejected text partition column" else - if grep -qi "cannot parse partition bound\|unsupported partition\|unsupported.*column\|not supported\|unrecognized\|parse.*bound" /tmp/journey-textpart.log; then + if grep -qi "cannot parse partition bound\|unsupported partition\|unsupported.*column\|not supported\|unrecognized\|parse.*bound" /tmp/journey-textpart-arch.log; thenAlso applies to: 3335-3335, 3369-3369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/journey.sh` at line 2942, Use a dedicated log file for each archive_only invocation in the affected TC-070, TC-071, and TC-072 stories instead of the register output log. Update the archive_only calls and their rejection greps so each assertion reads only the archive pass output, preserving the existing register logging and rejection patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ci/journey.sh`:
- Line 2942: Use a dedicated log file for each archive_only invocation in the
affected TC-070, TC-071, and TC-072 stories instead of the register output log.
Update the archive_only calls and their rejection greps so each assertion reads
only the archive pass output, preserving the existing register logging and
rejection patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96678383-3992-49b4-bfaa-4c224363db0c
📒 Files selected for processing (1)
ci/journey.sh
390a073 to
b5a1b90
Compare
2 participants
Added following test cases: