Contribution to Perldoc for TestLib module in Postgres

On 2019-Mar-19, Prajwal A V wrote:

I could not find the perldoc for TestLib module in src/test/perl and found
some difficultly in understanding what each function does while other
modules in the src/test folder had perldoc and it was easy to understand
the functions.

I would like to contribute for the perldoc for TestLib. I am looking for
your suggestions if this contribution is worth doing.

Yes, it is, please do.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 19, 2019 at 09:05:29AM -0300, Alvaro Herrera wrote:

Yes, it is, please do.

+1.
--
Michael

Hi,
Can I take this up?

Regards,
Ram

Sure, please go ahead.

Regards,
Prajwal.

On Thu, 21 Mar 2019, 19:11 Ramanarayana, <raam.soft@gmail.com> wrote:

Hi,

Please find the first version of the patch for review. I was not sure what
some of the functions are used for and marked them with TODO.

Cheers
Ram 4.0

On Fri, Mar 22, 2019 at 04:59:58PM +0530, Ramanarayana wrote:

Please find the first version of the patch for review. I was not sure what
some of the functions are used for and marked them with TODO.

This is only adding some documentation to an internal perl module we
ship, so it is far from being a critical part and we could still get
that into v12, still there are many patches waiting for integration
into v12 and this has showed up very late. Could you please register
this patch to the commit fest once you have a patch you think is fit
for merging? Here is the next commit fest link:
https://commitfest.postgresql.org/23/
--
Michael

Hi,
Please find the updated patch. Added to the commitfest as well
Regards,
Ram.

Hi Ram,

I think this documentation helps people who want to understand functions.

Please find the updated patch. Added to the commitfest as well

I have some comments.

I think some users who would like to know custom function check src/test/perl/README at first.
How about add comments to the README file, such as "See Testlib.pm if you want to know more details"?

In the above document, why not write a description after the function name?
I think it is better to write the function name first and then the description.
In your code;
#Checks if all the tests passed or not
all_tests_passing()

In my suggestion;
all_tests_passing()
Checks if all the tests passed or not

And some functions return value. How about adding return information to the above doc?

I find ^M character in your new code. Please use LF line ending not CRLF or get rid of it in next patch.

Regards,
Aya Iwata

On 7 Apr 2019, at 20:04, Ramanarayana <raam.soft@gmail.com> wrote:

Please find the updated patch. Added to the commitfest as well

The v2 patch is somewhat confused as it has Windows carriage returns rather
than newlines, so it replaces the entire file making the diff hard to read. It
also includes a copy of TestLib and the v1 patch and has a lot of whitespace
noise.

Please redo the patch on a clean tree to get a more easily digestable patch.

cheers ./daniel

On Tue, Jul 09, 2019 at 03:16:01PM +0200, Daniel Gustafsson wrote:

The v2 patch is somewhat confused as it has Windows carriage returns rather
than newlines, so it replaces the entire file making the diff hard to read. It
also includes a copy of TestLib and the v1 patch and has a lot of whitespace
noise.

Nobody can provide a clear review if the files are just fully
rewritten even based on a read of the patch. Perhaps you are working
on Windows and forgot to configure core.autocrlf with "git config".
That could make your life easier.

I have switched the patch as "waiting on author" for now.
--
Michael

On 2019-Apr-11, Iwata, Aya wrote:

In the above document, why not write a description after the function name?
I think it is better to write the function name first and then the description.
In your code;
#Checks if all the tests passed or not
all_tests_passing()

In my suggestion;
all_tests_passing()
Checks if all the tests passed or not

Yeah, so there are two parts in the submitted patch: first the synopsis
list the methods using this format you describe, and later the METHODS
section lists then again, using your suggested style. I think we should
do away with the long synopsis -- maybe keep it as just the "use
TestLib" line, and then let the METHODS section list and describe the
methods.

And some functions return value. How about adding return information
to the above doc?

That's already in the METHODS section for some of them. For example:

all_tests_passing()
Returns 1 if all the tests pass. Otherwise returns 0

It's missing for others, such as "tempdir".

In slurp_file you have this:
Opens the file provided as an argument to the function in read mode(as
indicated by <).
I think the parenthical comment is useless; remove that.

Please break long source lines (say to 78 chars -- make sure pgperltidy
agrees), and keep some spaces after sentence-ending periods and other
punctuation.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 7/10/19 9:38 AM, Alvaro Herrera wrote:

On 2019-Apr-11, Iwata, Aya wrote:

In the above document, why not write a description after the function name?
I think it is better to write the function name first and then the description.
In your code;
#Checks if all the tests passed or not
all_tests_passing()

In my suggestion;
all_tests_passing()
Checks if all the tests passed or not

Yeah, so there are two parts in the submitted patch: first the synopsis
list the methods using this format you describe, and later the METHODS
section lists then again, using your suggested style. I think we should
do away with the long synopsis -- maybe keep it as just the "use
TestLib" line, and then let the METHODS section list and describe the
methods.

And some functions return value. How about adding return information
to the above doc?

That's already in the METHODS section for some of them. For example:

all_tests_passing()
Returns 1 if all the tests pass. Otherwise returns 0

It's missing for others, such as "tempdir".

In slurp_file you have this:
Opens the file provided as an argument to the function in read mode(as
indicated by <).
I think the parenthical comment is useless; remove that.

Please break long source lines (say to 78 chars -- make sure pgperltidy
agrees), and keep some spaces after sentence-ending periods and other
punctuation.

I've fixed the bitrot and some other infelicities on this patch. It's
not commitable yet but I think it's more reviewable.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 26, 2019 at 09:51:34AM -0400, Andrew Dunstan wrote:

I've fixed the bitrot and some other infelicities on this patch. It's
not commitable yet but I think it's more reviewable.

Thanks, I had a look at this version.

+  # Returns the real directory for a virtual path directory under msys
+  real_dir(dir)
real_dir() is no more.

perl2host() is missing.

+  #TODO
+  command_like_safe(cmd, expected_stdout, test_name)
[...]
+=pod
+
+=item command_like_safe(cmd, expected_stdout, test_name)
+
+TODO
+
+=cut
Update not to miss.

+Runs the command which is passed as argument to the function. On failure it
+abandons further tests and exits the program.
"On failure the test suite exits immediately."

I think that the SYNOPSIS could be shaped better. As of now it is a
simple succession of the same commands listed without any link to each
other, which is contrary for example to what we do in PostgresNode.pm,
where it shows up a set of small examples which are useful to
understand how to shape the tests and the possible interactions
between the routines of the module. My take would be to keep it
simple and minimal as TestLib.pm is the lowest level of our TAP test
infrastructure. So here are some simple suggestions, and we could go
with this set to begin with:
# Test basic output of a command.
program_help_ok('initdb');
program_version_ok('initdb');
program_options_handling_ok('initdb');

# Test option combinations
command_fails(['initdb', '--invalid-option'],
'command fails with invalid option');
my $tempdir = TestLib::tempdir;
command_ok('initdb', '-D', $tempdir);

Another thing is that the examples should not overlap with what
PostgresNode.pm presents, and that it is not necessary to show up all
the routines. It also makes little sense to describe in the synopsis
the routines in a way which duplicates with the descriptions on top of
each routine.
--
Michael

On Tue, Jul 30, 2019 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Jul 26, 2019 at 09:51:34AM -0400, Andrew Dunstan wrote:

I've fixed the bitrot and some other infelicities on this patch. It's
not commitable yet but I think it's more reviewable.

Thanks, I had a look at this version.

[review listing things to fix]

Hi Ram,

Based on the above, it sounds like we want this patch but it needs a
bit more work. It's now the end of CF1. I'm moving this one to CF2
(September). Please post a new patch when ready.

Thanks!

--
Thomas Munro
https://enterprisedb.com

On 2019-Jul-30, Michael Paquier wrote:

I think that the SYNOPSIS could be shaped better. As of now it is a
simple succession of the same commands listed without any link to each
other, which is contrary for example to what we do in PostgresNode.pm,
where it shows up a set of small examples which are useful to
understand how to shape the tests and the possible interactions
between the routines of the module. My take would be to keep it
simple and minimal as TestLib.pm is the lowest level of our TAP test
infrastructure.

Agreed ... that's pretty much the same thing I tried to say upthread. I
wrote my own synopsis, partly using your suggestions. I reworded the
description for the routines (I don't think there's any I didn't touch),
added a mention of $windows_os, added a =head1 to split out the ad-hoc
routines from the Test::More wrappers.

And pushed.

Please give it another look. It might need more polish.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Sep 02, 2019 at 01:48:14PM -0400, Alvaro Herrera wrote:

Agreed ... that's pretty much the same thing I tried to say upthread. I
wrote my own synopsis, partly using your suggestions. I reworded the
description for the routines (I don't think there's any I didn't touch),
added a mention of $windows_os, added a =head1 to split out the ad-hoc
routines from the Test::More wrappers.

And pushed.

Please give it another look. It might need more polish.

Thanks for committing. I have read through the commit and I am not
noticing any issue sorting out. One thing may be to give a short
description for some of the routine's arguments like
check_mode_recursive, but I think that we could live without that
either.
--
Michael