Add additional information to src/test/ssl/README
Hi,
I've been trying to run the SSL tests against my branch and that was
tougher than expected because I didn't realize that additional output was
being saved that I couldn't see - it wasn't even getting to the part where
it could run the tests. This patch adds a note to the README explaining
where to look if you get an error.
Kevin
Attachments:
v1-0001-src-test-ssl-add-log-information-to-README.patchapplication/octet-stream; name=v1-0001-src-test-ssl-add-log-information-to-README.patchDownload
From d8fb484ea61653fdd7df899d2e13c1cd1a1b3d05 Mon Sep 17 00:00:00 2001
From: Kevin Burke <kevin@burke.dev>
Date: Fri, 29 Oct 2021 20:04:37 -0700
Subject: [PATCH v1] src/test/ssl: add log information to README
I didn't realize that there was additional information about my test
failure in the log directory, so let's try to make that more obvious.
---
src/test/ssl/README | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/test/ssl/README b/src/test/ssl/README
index f3f196e214..754670b9c3 100644
--- a/src/test/ssl/README
+++ b/src/test/ssl/README
@@ -89,6 +89,17 @@ recreate them if you need to make changes. "make sslfiles-clean" is required
in order to recreate the full set of keypairs and certificates. To rebuild
separate files, touch (or remove) the files in question and run "make sslfiles".
+Errors
+======
+
+If you encounter errors getting the test suite started, for example, output that
+looks like this:
+
+ t/001_ssltests.pl .. # Looks like your test exited with 29 before it could output anything.
+ t/001_ssltests.pl .. Dubious, test returned 29 (wstat 7424, 0x1d00)
+
+Additional information about the error may be present in the "log" directory.
+
TODO
====
--
2.33.1
Kevin Burke <kevin@burke.dev> writes:
I've been trying to run the SSL tests against my branch and that was
tougher than expected because I didn't realize that additional output was
being saved that I couldn't see - it wasn't even getting to the part where
it could run the tests. This patch adds a note to the README explaining
where to look if you get an error.
That's a generic thing about every one of our TAP tests, so documenting
it in that one README doesn't seem like an appropriate answer. Would
it have helped you to explain it in
https://www.postgresql.org/docs/devel/regress-tap.html
?
regards, tom lane
I probably would not have looked there honestly; I was working in the
terminal and had the source code right there. Could we put a link to that
page in the README?
"For more information on Postgres's TAP tests, see the docs:
https://www.postgresql.org/docs/devel/regress-tap.html"
Kevin
On Sat, Oct 30, 2021 at 7:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Kevin Burke <kevin@burke.dev> writes:
I've been trying to run the SSL tests against my branch and that was
tougher than expected because I didn't realize that additional output was
being saved that I couldn't see - it wasn't even getting to the partwhere
it could run the tests. This patch adds a note to the README explaining
where to look if you get an error.That's a generic thing about every one of our TAP tests, so documenting
it in that one README doesn't seem like an appropriate answer. Would
it have helped you to explain it inhttps://www.postgresql.org/docs/devel/regress-tap.html
?
regards, tom lane
Kevin Burke <kevin@burke.dev> writes:
I probably would not have looked there honestly; I was working in the
terminal and had the source code right there.
Yeah, that was kind of what I thought.
"For more information on Postgres's TAP tests, see the docs:
https://www.postgresql.org/docs/devel/regress-tap.html"
This doesn't seem tremendously useful, as we'd still have to duplicate
it everywhere. A quick search finds these TAP suites that have
associated README files:
$ for ft in `find . -name t`
do
d=`dirname $ft`
if [ -e $d/README ]; then
echo $d/README
fi
done
./src/bin/pg_amcheck/README
./src/test/authentication/README
./src/test/kerberos/README
./src/test/ldap/README
./src/test/modules/test_misc/README
./src/test/modules/test_pg_dump/README
./src/test/modules/libpq_pipeline/README
./src/test/recovery/README
./src/test/ssl/README
./src/test/subscription/README
The ones under modules/ can probably be excluded, as they're not
there to provide usage directions; but that still leaves us with
seven to touch. I'd be inclined to add just one sentence to the
boilerplate text these use, along the lines of
"If the tests fail, examining the logs left behind in tmp_check/log/
may be helpful."
regards, tom lane
On 30 Oct 2021, at 20:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'd be inclined to add just one sentence to the boilerplate text these use,
along the lines of
"If the tests fail, examining the logs left behind in tmp_check/log/
may be helpful."
Wouldn't it make more sense to start collecting troubleshooting advice in
src/test/perl/README and instead refer to that in the boilerplate? I notice
that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly
is my fault), a trubleshooting section in that file would be a good fit.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
Wouldn't it make more sense to start collecting troubleshooting advice in
src/test/perl/README and instead refer to that in the boilerplate? I notice
that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly
is my fault), a trubleshooting section in that file would be a good fit.
Yeah, we could try to move all the repetitive stuff to there. I was a bit
allergic to the idea of having README files refer to webpages, because you
might be offline --- but referencing a different README doesn't have that
issue.
regards, tom lane
I wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Wouldn't it make more sense to start collecting troubleshooting advice in
src/test/perl/README and instead refer to that in the boilerplate? I notice
that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly
is my fault), a trubleshooting section in that file would be a good fit.
Yeah, we could try to move all the repetitive stuff to there. I was a bit
allergic to the idea of having README files refer to webpages, because you
might be offline --- but referencing a different README doesn't have that
issue.
Here's a quick stab at rearranging src/test/perl/README so that the
initial section is all how-to-run-the-tests info, and advice about
writing new tests starts after that. Your point about PG_TEST_NOCLEAN
could be added into the initial section.
I'd envision adding something like
"See src/test/perl/README for more info about running these tests."
to the boilerplate "Running the tests" section in src/test/ssl/README
and its cohorts, but I didn't bother with that yet.
regards, tom lane
Attachments:
readme-improvements-wip.patchtext/x-diff; charset=us-ascii; name=readme-improvements-wip.patchDownload
diff --git a/src/test/perl/README b/src/test/perl/README
index 0e9a00ea05..f75b224175 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -12,22 +12,29 @@ $(prove_installcheck) targets in Makefile.global. By default every test in the
t/ subdirectory is run. Individual test(s) can be run instead by passing
something like PROVE_TESTS="t/001_testname.pl t/002_othertestname.pl" to make.
-You should prefer to write tests using pg_regress in src/test/regress, or
-isolation tester specs in src/test/isolation, if possible. If not, check to
-see if your new tests make sense under an existing tree in src/test, like
-src/test/ssl, or should be added to one of the suites for an existing utility.
-
-Note that all tests and test tools should have perltidy run on them before
-patches are submitted, using perltidy --profile=src/tools/pgindent/perltidyrc
-
By default, to keep the noise low during runs, we do not set any flags via
PROVE_FLAGS, but this can be done on the 'make' command line if desired, eg:
make check-world PROVE_FLAGS='--verbose'
+When a test fails, the terminal output from 'prove' is usually not sufficient
+to diagnose the problem. Look into the log files that are left under
+tmp_check/log/ to get more info. Files named 'regress_XXX' are log output
+from the perl test scripts themselves, and should be examined first.
+Other files are postmaster logs, and may be helpful as additional data.
+
+
Writing tests
-------------
+You should prefer to write tests using pg_regress in src/test/regress, or
+isolation tester specs in src/test/isolation, if possible. If not, check to
+see if your new tests make sense under an existing tree in src/test, like
+src/test/ssl, or should be added to one of the suites for an existing utility.
+
+Note that all tests and test tools should have perltidy run on them before
+patches are submitted, using perltidy --profile=src/tools/pgindent/perltidyrc
+
Tests are written using Perl's Test::More with some PostgreSQL-specific
infrastructure from src/test/perl providing node management, support for
invoking 'psql' to run queries and get results, etc. You should read the
On 31 Oct 2021, at 19:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Wouldn't it make more sense to start collecting troubleshooting advice in
src/test/perl/README and instead refer to that in the boilerplate? I notice
that we don't document for example PG_TEST_NOCLEAN anywhere (which admittedly
is my fault), a trubleshooting section in that file would be a good fit.Yeah, we could try to move all the repetitive stuff to there. I was a bit
allergic to the idea of having README files refer to webpages, because you
might be offline --- but referencing a different README doesn't have that
issue.Here's a quick stab at rearranging src/test/perl/README so that the
initial section is all how-to-run-the-tests info, and advice about
writing new tests starts after that. Your point about PG_TEST_NOCLEAN
could be added into the initial section.
That's pretty much just what I was thinking, definitely +1 on this patch.
I'd envision adding something like
"See src/test/perl/README for more info about running these tests."
to the boilerplate "Running the tests" section in src/test/ssl/README
and its cohorts, but I didn't bother with that yet.
Sounds good.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
On 31 Oct 2021, at 19:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Here's a quick stab at rearranging src/test/perl/README so that the
initial section is all how-to-run-the-tests info, and advice about
writing new tests starts after that. Your point about PG_TEST_NOCLEAN
could be added into the initial section.
That's pretty much just what I was thinking, definitely +1 on this patch.
Done that way; feel free to add more material to perl/README.
regards, tom lane
The patch looks great. Thanks!
Kevin
On Sun, Oct 31, 2021 at 3:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Daniel Gustafsson <daniel@yesql.se> writes:
On 31 Oct 2021, at 19:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Here's a quick stab at rearranging src/test/perl/README so that the
initial section is all how-to-run-the-tests info, and advice about
writing new tests starts after that. Your point about PG_TEST_NOCLEAN
could be added into the initial section.That's pretty much just what I was thinking, definitely +1 on this patch.
Done that way; feel free to add more material to perl/README.
regards, tom lane
On 31 Oct 2021, at 23:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Done that way; feel free to add more material to perl/README.
Attached is a small addition mentioning PG_TEST_NOCLEAN
--
Daniel Gustafsson https://vmware.com/
Attachments:
readme_retaindir.diffapplication/octet-stream; name=readme_retaindir.diff; x-unix-mode=0644Download
diff --git a/src/test/perl/README b/src/test/perl/README
index d62db9b5d8..b02e1ffc3d 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -23,6 +23,11 @@ tmp_check/log/ to get more info. Files named 'regress_log_XXX' are log
output from the perl test scripts themselves, and should be examined first.
Other files are postmaster logs, and may be helpful as additional data.
+Data directories will also be left behind for analysis when a test fails,
+and are named according to the test filename. When the environment variable
+PG_TEST_NOCLEAN is set, data directories will be retained regardless of test
+status.
+
Writing tests
-------------
Daniel Gustafsson <daniel@yesql.se> writes:
Attached is a small addition mentioning PG_TEST_NOCLEAN
Maybe could use a bit of copy-editing, say
Data directories will also be left behind for analysis when a test fails;
they are named according to the test filename. But if the environment
variable PG_TEST_NOCLEAN is set, data directories will be retained
regardless of test status.
regards, tom lane
On 12 Nov 2021, at 16:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
Attached is a small addition mentioning PG_TEST_NOCLEAN
Maybe could use a bit of copy-editing, say
Data directories will also be left behind for analysis when a test fails;
they are named according to the test filename. But if the environment
variable PG_TEST_NOCLEAN is set, data directories will be retained
regardless of test status.
Agreed, that reads better. Pushed with those changes.
--
Daniel Gustafsson https://vmware.com/