Обсуждение: Add additional information to src/test/ssl/README

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

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

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

> 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/

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/