tests: refactor oriole pg_regress and add Docker regress coverage #2011
Conversation
WalkthroughAdds OrioleDB-aware testing and docs: patches Dockerfile config, extends Nix test harness to detect/filter OrioleDB tests and preload the extension, adds many OrioleDB SQL tests, supplies a Bash tool to run pg_regress in Docker, and adds comprehensive Docker-testing documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as test-docker-image.sh
participant Docker as Docker Engine
participant Container as Postgres Container
participant Nix as Nix/Binaries
participant PgRegress as pg_regress
User->>Script: run with Dockerfile & options
Script->>Docker: build image (unless --no-build)
Docker-->>Script: image available
Script->>Docker: start container with env/ports
Docker->>Container: launch Postgres
Container-->>Script: container running
Script->>Container: wait for pg_isready (retry)
Container-->>Script: ready
Script->>Nix: fetch psql & pg_regress for version
Nix-->>Script: binaries returned
Script->>Container: start mock server / set test config
Script-->>Script: collect & filter tests (OrioleDB-aware)
Script-->>Script: patch expected outputs for Docker
Script->>PgRegress: invoke with test list
PgRegress->>Container: run tests
Container-->>PgRegress: test results
PgRegress-->>Script: report & outputs
Script->>Docker: cleanup container (unless --keep)
Script->>User: display summary / failures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@nix/tests/sql/z_orioledb-17_docs-functions.sql`:
- Around line 66-76: The function hello_world_definer is discarding its SELECT
result and thus returns NULL; update the function body to explicitly return the
value (e.g., use RETURN 'hello world'; or SELECT 'hello world' INTO a variable
and then RETURN that variable) so the function actually returns the expected
text from the plpgsql block.
In `@nix/tests/sql/z_orioledb-17_pgroonga.sql`:
- Around line 9-26: The query fails because after the CTE tokenizers already
returns a single JSON value, calling jsonb_array_elements on (select * from
tokenizers) tries to treat that object as an array; replace the second
jsonb_array_elements usage by selecting directly from the tokenizers CTE and
extracting the 'name' field, i.e. iterate over tokenizers rows (CTE
"tokenizers") instead of invoking jsonb_array_elements a second time; update
references where t.x::jsonb ->> 'name' is read to source from tokenizers to
avoid the "cannot extract elements from an object" error.
In `@test-docker-image.sh`:
- Around line 379-386: The hardcoded sed substitution of OID "14007" -> "13638"
is fragile; replace it by dynamically discovering the actual OID at runtime and
using that value in the replacement (rather than fixed literals) — in the
test-docker-image.sh block that references PATCHED_TESTS_DIR and the expected
file expected/z_orioledb-17_index_advisor.out, run a short psql query to fetch
the catalog OID you care about (e.g., query pg_class or the specific object name
used by the index advisor) into a shell variable (e.g., INDEX_ADVISOR_OID) and
then use sed to substitute that variable into the expected file; alternatively,
if you prefer comparison-time flexibility, change the test harness that compares
expected/z_orioledb-17_index_advisor.out to accept a regex or normalize OIDs
before diffing so you no longer rely on the literal 14007/13638 replacement.
🧹 Nitpick comments (6)
nix/docs/docker-testing.md (1)
29-41: Add language identifier to fenced code block.The usage synopsis code block is missing a language identifier, which helps with syntax highlighting and accessibility.
Suggested fix
-``` +```text Usage: ./test-docker-image.sh [OPTIONS] DOCKERFILEnix/checks.nix (2)
131-137: Extract magic numbers for substring operation.The substring operation uses magic numbers
14and18representing the prefix length and total removal length. Consider defining these as named values for clarity.Suggested improvement
# Get list of OrioleDB-specific test basenames (without z_orioledb-17_ prefix) orioledbPrefix = "z_orioledb-17_"; orioledbPrefixLen = builtins.stringLength orioledbPrefix; # 14 sqlSuffixLen = 4; # ".sql" orioledbVariants = pkgs.lib.pipe files [ builtins.attrNames (builtins.filter (n: builtins.match "z_orioledb-17_.*\\.sql" n != null)) (map (n: builtins.substring orioledbPrefixLen (pkgs.lib.stringLength n - orioledbPrefixLen - sqlSuffixLen) n)) ];
291-298: OrioleDB detection relies on underscore in version string.The check
[[ "${pgpkg.version}" == *"_"* ]]to detect OrioleDB is fragile and could match unintended version strings. This pattern is repeated at lines 293, 327, and 373. Consider using a more explicit check or extracting a helper variable.Suggested approach
Define a variable at the top of the script section:
IS_ORIOLEDB=$([[ "${pgpkg.version}" == *"_"* ]] && echo "true" || echo "false")Then use
[[ "$IS_ORIOLEDB" == "true" ]]in subsequent checks, or use a more explicit pattern like17_*to avoid false positives from potential future version formats.test-docker-image.sh (2)
311-316: Replacesleep 2with readiness check for HTTP mock server.The fixed 2-second sleep is unreliable. Consider polling for the mock server's availability:
Suggested improvement
# Start mock server in container background docker exec -d "$CONTAINER_NAME" python3 /tmp/http-mock-server.py 8880 HTTP_MOCK_PORT=8880 - # Wait for mock server to be ready - sleep 2 + # Wait for mock server to be ready + local mock_attempts=10 + for ((i=1; i<=mock_attempts; i++)); do + if docker exec "$CONTAINER_NAME" curl -s -o /dev/null -w '' "http://localhost:$HTTP_MOCK_PORT/" 2>/dev/null; then + break + fi + if [[ $i -eq $mock_attempts ]]; then + log_warn "HTTP mock server may not be ready" + fi + sleep 0.5 + done log_info "HTTP mock server started on port $HTTP_MOCK_PORT (inside container)"
129-141: Potential empty array issue in orioledb_variants loop.If no OrioleDB variant files exist, the glob
"$TESTS_SQL_DIR"/z_orioledb-17_*.sqlmay expand to the literal pattern (depending on shell options), causing the-fcheck to fail silently. The array iteration at line 129 will also fail iforioledb_variantsis empty.Suggested fix
+ # Skip variant check if no variants exist + if [[ ${`#orioledb_variants`[@]} -eq 0 ]]; then + tests+=("$basename") + else # Skip common test if OrioleDB-specific variant exists local has_variant=false for variant in "${orioledb_variants[@]}"; do if [[ "$basename" == "$variant" ]]; then has_variant=true break fi done if [[ "$has_variant" == "false" ]]; then tests+=("$basename") fi + fiOr add
shopt -s nullglobbefore the glob expansion at line 96.nix/tests/sql/z_orioledb-17_docs-functions.sql (1)
149-162: Consider usingPERFORMfor discarded query results.While the bare
SELECTworks to trigger the error,PERFORMis the idiomatic plpgsql construct for executing queries when the result is intentionally discarded.♻️ Optional style improvement
create function error_example() returns void language plpgsql as $$ begin - select * from table_that_does_not_exist; + perform * from table_that_does_not_exist; exception when others then raise exception 'An error occurred in function <function name>: %', sqlerrm; end; $$;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/checks.nix`:
- Around line 146-163: The skip check uses basename (e.g.
"z_orioledb-17_index_advisor") directly so entries in orioledbSkipTests (like
"index_advisor") don't match; create a normalized name that strips the
version-specific prefix before checking. Add something like normalizedBasename =
if isVersionSpecific then builtins.replaceStrings [ "z_orioledb-17_" "z_17_"
"z_15_" ] [ "" "" "" ] basename else basename and then use normalizedBasename in
isSkippedForOrioledb (and anywhere else you compare against orioledbSkipTests or
hasOrioledbVariant) so OrioleDB-specific filenames are properly excluded.
In `@test-docker-image.sh`:
- Around line 115-141: The OrioleDB skip check compares the full test basename
(e.g., z_orioledb-17_index_advisor) against entries in ORIOLEDB_SKIP_TESTS
(which are plain names like index_advisor), so OrioleDB-specific tests are not
being skipped; modify the skip logic around the ORIOLEDB_SKIP_TESTS loop to
first strip the z_orioledb-17_ (or more generally the z_*_ prefix) from basename
when version == "orioledb-17" and then compare the stripped name to
ORIOLEDB_SKIP_TESTS; update the variables referenced (basename,
ORIOLEDB_SKIP_TESTS, should_skip) and ensure the existing continue behavior
remains when a match is found.
♻️ Duplicate comments (1)
test-docker-image.sh (1)
337-349: Create the OrioleDB extension before running prime.sql to match the Nix harness.The Nix checks create the
orioledbextension before runningprime.sql(see nix/checks.nix lines 334-339), but the Docker flow does not. Sinceprime.sqldoes not include OrioleDB, tests likeverify_orioledband others requiring OrioleDB will fail when running withVERSION=orioledb-17. Add the extension creation before running prime.sql to align with the Nix behavior.🔧 Suggested fix
# Run prime.sql to enable extensions log_info "Running prime.sql to enable extensions..." + if [[ "$VERSION" == "orioledb-17" ]]; then + log_info "Creating orioledb extension..." + PGPASSWORD="$POSTGRES_PASSWORD" "$PSQL_PATH" \ + -h localhost \ + -p "$PORT" \ + -U "$POSTGRES_USER" \ + -d "$POSTGRES_DB" \ + -c "CREATE EXTENSION IF NOT EXISTS orioledb;" + fi if ! PGPASSWORD="$POSTGRES_PASSWORD" "$PSQL_PATH" \ -h localhost \ -p "$PORT" \
samrose
force-pushed
the
oriole-docker-tests
branch
from
fceb2f5 to
e7ca5b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/docs/docker-testing.md`:
- Around line 102-107: Update the GitHub Actions step that currently says "uses:
cachix/install-nix-action@v24" to a current tag (e.g., "v31" or a recent full
release like "v31.8.4") so the Docker test workflow uses the latest
cachix/install-nix-action; locate the "Test Docker image" workflow job step
where the "uses: cachix/install-nix-action@v24" line appears and replace the
version tag accordingly.
♻️ Duplicate comments (1)
test-docker-image.sh (1)
115-127: Skip list comparison doesn't handle OrioleDB-prefixed test names.Similar to the issue in
nix/checks.nix, the skip check compares the fullbasename(e.g.,z_orioledb-17_index_advisor) againstORIOLEDB_SKIP_TESTSwhich contains unprefixed names like"index_advisor". Strip the prefix before comparing.🔧 Suggested fix
# Skip tests that don't work with OrioleDB if [[ "$version" == "orioledb-17" ]]; then local should_skip=false + # Normalize basename for skip check (strip z_orioledb-17_ prefix if present) + local check_name="$basename" + if [[ "$basename" == z_orioledb-17_* ]]; then + check_name="${basename#z_orioledb-17_}" + fi for skip_test in "${ORIOLEDB_SKIP_TESTS[@]}"; do - if [[ "$basename" == "$skip_test" ]]; then + if [[ "$check_name" == "$skip_test" ]]; then should_skip=true break fi done
🧹 Nitpick comments (5)
nix/docs/docker-testing.md (1)
29-41: Add language specifier to fenced code block.The usage block should have a language specifier for proper rendering and consistency with other code blocks in the document.
📝 Suggested fix
-``` +```text Usage: ./test-docker-image.sh [OPTIONS] DOCKERFILEnix/checks.nix (3)
146-153: Skip list comparison still uses prefixed basename.The skip check compares the full
basename(e.g.,z_orioledb-17_index_advisor) againstorioledbSkipTestswhich contains unprefixed names like"index_advisor". This means OrioleDB-specific test files matching skip patterns won't be excluded.However, looking at the broader logic: version-specific files (
z_orioledb-17_*) are handled at lines 154-159, andisSkippedForOrioledbonly affects whether the test runs at all. For common tests (non-z_prefixed), the basename would beindex_advisorwhich would match. The OrioleDB-specific variantz_orioledb-17_index_advisorwould still run if it exists.If the intent is to also skip
z_orioledb-17_index_advisor.sql, normalize the basename before the skip check:🔧 Suggested fix
basename = builtins.substring 0 (pkgs.lib.stringLength name - 4) name; # Remove .sql + # Normalize OrioleDB-prefixed names for skip check + normalizedBasename = if builtins.match "z_orioledb-17_.*" basename != null + then builtins.substring 14 (pkgs.lib.stringLength basename - 14) basename + else basename; # Skip tests that don't work with OrioleDB - isSkippedForOrioledb = version == "orioledb-17" && builtins.elem basename orioledbSkipTests; + isSkippedForOrioledb = version == "orioledb-17" && builtins.elem normalizedBasename orioledbSkipTests;
300-307: OrioleDB detection relies on underscore convention.The detection
[[ "${pgpkg.version}" == *"_"* ]]works for current OrioleDB versions like17_15, but is an implicit convention. Consider adding a comment explaining this dependency on the version format, or using a more explicit check if themajorVersioncould be passed to the shell script.
77-88: Consider removing debug trace statements.The
builtins.tracecalls on lines 77 and 88 will output debug information during Nix evaluation. If these were added for debugging, consider removing them before merging to reduce noise in production builds.🧹 Suggested cleanup
majorVersion = let - version = builtins.trace "pgpkg.version is: ${pgpkg.version}" pgpkg.version; + version = pgpkg.version; isOrioledbMatch = builtins.match "^17_[0-9]+$" version != null; isSeventeenMatch = builtins.match "^17[.][0-9]+$" version != null; result = if isOrioledbMatch then "orioledb-17" else if isSeventeenMatch then "17" else "15"; in - builtins.trace "Major version result: ${result}" result; + result;test-docker-image.sh (1)
330-335: Consider adding retry logic for HTTP mock server readiness.The
sleep 2is a fixed wait that may be insufficient on slower systems or excessive on fast ones. Consider adding a retry loop similar towait_for_postgresto check if the mock server is actually ready (e.g., viacurlornc).💡 Example improvement
# Start mock server in container background docker exec -d "$CONTAINER_NAME" python3 /tmp/http-mock-server.py 8880 HTTP_MOCK_PORT=8880 - # Wait for mock server to be ready - sleep 2 + # Wait for mock server to be ready (retry up to 10 times) + local mock_attempt=1 + while [[ $mock_attempt -le 10 ]]; do + if docker exec "$CONTAINER_NAME" curl -s http://localhost:8880/ >/dev/null 2>&1; then + break + fi + sleep 1 + ((mock_attempt++)) + done + if [[ $mock_attempt -gt 10 ]]; then + log_warn "HTTP mock server may not be ready" + fi log_info "HTTP mock server started on port $HTTP_MOCK_PORT (inside container)"
Co-authored-by: Douglas J Hunley <[email protected]>
Co-authored-by: Douglas J Hunley <[email protected]>
Co-authored-by: Douglas J Hunley <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test-docker-image.sh`:
- Around line 192-212: The wait_for_postgres function currently calls pg_isready
on the host; change it to run pg_isready inside the container via docker exec to
avoid requiring local postgres client tools. Update wait_for_postgres to accept
(or use the existing) container identifier and replace the pg_isready invocation
with something like docker exec "$container" pg_isready -h "$host" -p "$port" -U
"$POSTGRES_USER" -q (preserving exit handling and logging in the function), and
apply the same container-side check to the other occurrence referenced (around
lines 291-296).
- Around line 129-141: When version is "orioledb-17" the case block should try
adding z_orioledb-17_* tests but fall back to z_17_* when no OrioleDB-specific
file exists; modify the "orioledb-17" branch in the case on "$version" (the
block that inspects "$basename" and appends to tests[]) so it first checks [[
"$basename" == z_orioledb-17_* ]] && tests+=("$basename") and if that fails also
allow [[ "$basename" == z_17_* ]] && tests+=("$basename") (mirroring the
non-version-specific fallback logic used elsewhere).
when variants exist
- docs-cascades-deletes, docs-full-text-search, docs-functions, docs-indexes, docs-partitioning - Tests with different output due to
orioledb storage
- extensions_schema - Shows orioledb in extension list
- index_advisor - Different error OID with orioledb
- pgroonga - Different output with orioledb
- verify_orioledb - New test that explicitly verifies orioledb is loaded and working
Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.