-
Notifications
You must be signed in to change notification settings - Fork 45
Improve TAP test infrastructure #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds TAP regression scheduling and structured test logging: Makefile gains TESTLOGDIR, SCHEDULE_FILE, and TAPS_REGRESS with updated prove_check; test runner exports TESTLOGDIR; SpockTest.pm derives per-test log names, creates log dirs, hardens psql environment, and passes -X to psql. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Makefile`:
- Around line 28-29: Remove the unused cleanup path 'tests/tap/t/logs' from the
EXTRA_CLEAN variable in the Makefile: edit the line that defines EXTRA_CLEAN
(which currently includes "$(control_path) spock_compat.bc \ tests/logs
tests/tap/logs tests/tap/t/logs") and delete the 'tests/tap/t/logs' entry so
only actual log directories (e.g. tests/logs and tests/tap/logs) remain.
🧹 Nitpick comments (1)
Makefile (1)
107-109:TESTLOGDIRis created but not used by tests.The
TESTLOGDIRvariable is set and the directory is created (lines 112-113), but it's not passed to the tests.SpockTest.pmcreates its ownlogs/directory in the working directory and doesn't referenceTESTLOGDIR.If the intent is to centralize logs to
TESTLOGDIR, consider passing it via environment variable:🔧 Suggested enhancement
cd $(srcdir)/tests/tap && \ TESTDIR='$(CURDIR)' \ + SPOCKTEST_LOG_DIR='$(TESTLOGDIR)' \ PATH="$(shell $(PG_CONFIG) --bindir):$$PATH" \And update
SpockTest.pmto use it:my $log_dir = $ENV{SPOCKTEST_LOG_DIR} // "logs"; my $LOG_FILE = $ENV{SPOCKTEST_LOG_FILE} // "${log_dir}/${test_name}.log"; eval { make_path($log_dir) };
Makefile changes: - Add TESTLOGDIR variable for centralized log location - Parse test schedule file to run only configured tests - Add PATH setup with pg_config bindir - Add --timer flag to show test execution times - Clean and recreate log directory before each run - Add test log directories to EXTRA_CLEAN SpockTest.pm changes: - Derive log filename from test name instead of PID - Use File::Basename to extract test name from script path - Log files now named as 001_basic.log, 002_create_subscriber.log, etc. - Makes it easier to identify which log belongs to which test - Fix psqlrc interference by setting PSQLRC=/dev/null and adding -X flag - Centralize logs using TESTLOGDIR environment variable Co-Authored-By: Claude Opus 4.5 <[email protected]>
2153377 to
b8ce09d
Compare
ibrarahmad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Makefile changes:
SpockTest.pm changes:
Result: Better organized test logs and real-time test progress visibility