Spoc-505: Fix Codacy issues and rewrite golang tests with the built-in PostgreSQL infrastructure#36
Spoc-505: Fix Codacy issues and rewrite golang tests with the built-in PostgreSQL infrastructure#36mason-sharp merged 3 commits intomainfrom
Conversation
The Go-based test suite covered three large-object operations — lo_lseek, lo_tell, and lo_truncate — that had no equivalent in the SQL regression or TAP tests. Add coverage for all three before the Go tests are removed. sql/lolor.sql and expected/lolor.out: three new sections exercise lo_lseek (seek to offset 15, overwrite 11 bytes, read back and verify the splice), lo_tell (confirm cursor is at 0 on a fresh open and at 11 after a lowrite of 11 bytes), and lo_truncate (write 36 bytes, truncate to 10, verify only the prefix survives). t/005_logical_replication.pl: extend the logical-replication scenario with the same three operations on the publisher side, wait for subscriber catchup after each mutating step, and assert the correct content on both nodes. Also add catalog-consistency checks that confirm lolor.pg_largeobject_metadata and lolor.pg_largeobject row counts match between publisher and subscriber after all operations.~
The Go tests covered lo_lseek, lo_tell, and lo_truncate operations that were not tested elsewhere. Those gaps have now been filled by SQL regression tests and TAP tests, making the Go suite redundant. Remove tests/go/ (main_test.go, go.mod, test.properties, README), docker/test-configs/test.properties.go, the Golang section from docker/run-tests.sh, and replace the golang:1.22.4 Docker base image in docker/Dockerfile.tester with debian:bookworm, which provides the same OS environment without the Go toolchain.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request removes the Go-based test suite and migrates testing to SQL and Perl-based approaches. The changes eliminate Go toolchain dependencies, remove Go test files and configuration, and add comprehensive SQL regression checks for large-object operations with logical replication coverage. Changes
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 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
|
| Category | Results |
|---|---|
| Compatibility | 5 high |
TIP This summary will be updated as you push new changes. Give us feedback
![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
🧹 Nitpick comments (1)
docker/Dockerfile.tester (1)
11-19: Consider running the container as a non-root user.The Dockerfile creates the
pgedgeuser but never switches to it. The container will run as root, which violates security best practices. Since this user is already created and the test scripts appear to work under/home/pgedge, adding aUSERdirective should be straightforward.🔒 Proposed fix to run as non-root user
WORKDIR /home/pgedge/lolor/tests +USER pgedge CMD /home/pgedge/run-tests.shStatic analysis hint: Trivy DS-0002 - "Image user should not be 'root'. Specify at least 1 USER command in Dockerfile with non-root user as argument."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile.tester` around lines 11 - 19, The Dockerfile creates the pgedge user and copies run-tests.sh but never switches away from root; update the Dockerfile to run the container as the non-root user by adding a USER pgedge directive after setting ownership/permissions (ensure /home/pgedge/run-tests.sh is chowned to pgedge and made executable and the WORKDIR /home/pgedge/lolor/tests is owned by pgedge) so that CMD /home/pgedge/run-tests.sh executes as the pgedge user rather than root; reference the pgedge user, run-tests.sh, WORKDIR and CMD instructions when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@t/005_logical_replication.pl`:
- Around line 103-129: The test currently writes 11 bytes to large object 4 on
$publisher using lowrite but never waits for replication or asserts the
subscriber state; add a replication wait and a subscriber-side assertion: after
the publisher block that calls lowrite(:fd) and lo_close(:fd), call
wait_for_catchup(...) to ensure the subscriber has applied the change, then run
the same lo_open/lo_tell/lo_close sequence against $subscriber (using
$subscriber->safe_psql) and assert the lo_tell result is '11'. Reference the
existing symbols: $publisher, $subscriber, lowrite, lo_tell, lo_open, lo_close,
and wait_for_catchup to locate where to insert the wait and the subscriber
verification.
---
Nitpick comments:
In `@docker/Dockerfile.tester`:
- Around line 11-19: The Dockerfile creates the pgedge user and copies
run-tests.sh but never switches away from root; update the Dockerfile to run the
container as the non-root user by adding a USER pgedge directive after setting
ownership/permissions (ensure /home/pgedge/run-tests.sh is chowned to pgedge and
made executable and the WORKDIR /home/pgedge/lolor/tests is owned by pgedge) so
that CMD /home/pgedge/run-tests.sh executes as the pgedge user rather than root;
reference the pgedge user, run-tests.sh, WORKDIR and CMD instructions when
applying the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61b02868-7a57-4ce6-a6fb-8bc8bed5effc
⛔ Files ignored due to path filters (1)
expected/lolor.outis excluded by!**/*.out
📒 Files selected for processing (9)
docker/Dockerfile.testerdocker/run-tests.shdocker/test-configs/test.properties.gosql/lolor.sqlt/005_logical_replication.pltests/go/README.mdtests/go/go.modtests/go/main_test.gotests/go/test.properties
💤 Files with no reviewable changes (6)
- tests/go/test.properties
- docker/run-tests.sh
- docker/test-configs/test.properties.go
- tests/go/go.mod
- tests/go/README.md
- tests/go/main_test.go
2 participants
Purpose
Branch
spoc-505removes the Go test suite from lolor as part of theongoing effort to eliminate Go infrastructure from the repository. Before
anything could be deleted, the three large-object operations that only the
Go tests exercised —
lo_lseek,lo_tell, andlo_truncate— neededcoverage in the native PostgreSQL test suite.
Changes
Add regression and TAP tests for lo_lseek, lo_tell, lo_truncate
sql/lolor.sqlandexpected/lolor.outgain three new sections thatexercise each operation in isolation: seeking to an offset and overwriting
partial content, tracking cursor position before and after a write, and
truncating an object to a shorter length.
t/005_logical_replication.plis extended with the same three operationsin a publisher/subscriber scenario. After each mutating step the test
waits for subscriber catchup and asserts the correct content on both nodes.
Two catalog-consistency checks are also added, confirming that
lolor.pg_largeobject_metadataandlolor.pg_largeobjectrow counts matchbetween nodes after all operations.
Remove Go test suite and infrastructure
tests/go/(main_test.go, go.mod, test.properties, README),docker/test-configs/test.properties.go, and the Golang section ofdocker/run-tests.share deleted. Thegolang:1.22.4base image indocker/Dockerfile.testeris replaced withdebian:bookworm, whichprovides the same OS environment without the Go toolchain.