Обсуждение: can we add subscription TAP test option "vcregress subscriptioncheck" for MSVC builds?

Hi,

While working one of the internal features, I figured out that we
don't have subscription TAP tests option for "vcregress" tool for msvc
builds. Is there any specific reason that we didn't add "vcregress
subscriptioncheck" option similar to "vcregress recoverycheck"? It
looks like one can run with "vcregress taptest" option and PROVE_FLAGS
environment variable(I haven't tried it myself), but having
subscriptioncheck makes life easier.

Here's a small patch for that. Thoughts?

Regards,
Bharath Rupireddy.

On 10/5/21 1:25 PM, Bharath Rupireddy wrote:
> Hi,
>
> While working one of the internal features, I figured out that we
> don't have subscription TAP tests option for "vcregress" tool for msvc
> builds. Is there any specific reason that we didn't add "vcregress
> subscriptioncheck" option similar to "vcregress recoverycheck"? It
> looks like one can run with "vcregress taptest" option and PROVE_FLAGS
> environment variable(I haven't tried it myself), but having
> subscriptioncheck makes life easier.
>
> Here's a small patch for that. Thoughts?
>


I would actually prefer to reduce the number of special things in
vcregress.pl rather than add more. We should be able to add a new set of
TAP tests somewhere without having to do anything to vcregress.pl.
That's more or less why I added the taptest option in the first place.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2021-10-05 16:03:53 -0400, Andrew Dunstan wrote:
> I would actually prefer to reduce the number of special things in
> vcregress.pl rather than add more. We should be able to add a new set of
> TAP tests somewhere without having to do anything to vcregress.pl.
> That's more or less why I added the taptest option in the first place.

My problem with that is that that means there's no convenient way to discover
what one needs to do to run all tests. Perhaps we could have one all-taptest
target or such?

Greetings,

Andres Freund

On 10/5/21 4:38 PM, Andres Freund wrote:
> Hi,
>
> On 2021-10-05 16:03:53 -0400, Andrew Dunstan wrote:
>> I would actually prefer to reduce the number of special things in
>> vcregress.pl rather than add more. We should be able to add a new set of
>> TAP tests somewhere without having to do anything to vcregress.pl.
>> That's more or less why I added the taptest option in the first place.
> My problem with that is that that means there's no convenient way to discover
> what one needs to do to run all tests. Perhaps we could have one all-taptest
> target or such?
>

Yeah. That's a much better proposal.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, Oct 05, 2021 at 06:18:47PM -0400, Andrew Dunstan wrote:
> On 10/5/21 4:38 PM, Andres Freund wrote:
>> My problem with that is that that means there's no convenient way to discover
>> what one needs to do to run all tests. Perhaps we could have one all-taptest
>> target or such?
>>
>
> Yeah. That's a much better proposal.

+1.  It is so easy to forget one or more targets when running this
script.
--
Michael

On 2021-Oct-06, Michael Paquier wrote:

> On Tue, Oct 05, 2021 at 06:18:47PM -0400, Andrew Dunstan wrote:
> > On 10/5/21 4:38 PM, Andres Freund wrote:
> >> My problem with that is that that means there's no convenient way to discover
> >> what one needs to do to run all tests. Perhaps we could have one all-taptest
> >> target or such?
> > 
> > Yeah. That's a much better proposal.

So how about "vcregress check-world"?

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

On Wed, Oct 6, 2021 at 6:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Oct-06, Michael Paquier wrote:
>
> > On Tue, Oct 05, 2021 at 06:18:47PM -0400, Andrew Dunstan wrote:
> > > On 10/5/21 4:38 PM, Andres Freund wrote:
> > >> My problem with that is that that means there's no convenient way to discover
> > >> what one needs to do to run all tests. Perhaps we could have one all-taptest
> > >> target or such?
> > >
> > > Yeah. That's a much better proposal.
>
> So how about "vcregress check-world"?

I was thinking of the same. +1 for "vcregress check-world" which is
more in sync with it's peer "make check-world" instead of "vcregress
all-taptest". I'm not sure whether we can also have "vcregress
installcheck-world" as well.

Having said that, with these new options, are we going to have only below?

vcregress check
vcregress installcheck
vcregress check-world
vcregress installcheck-world (?)

And remove others?

vcregress plcheck
vcregress contribcheck
vcregress modulescheck
vcregress ecpgcheck
vcregress isolationcheck
vcregress bincheck
vcregress recoverycheck
vcregress upgradecheck

Regards,
Bharath Rupireddy.

On Wed, Oct 06, 2021 at 07:19:04AM +0530, Bharath Rupireddy wrote:
> I was thinking of the same. +1 for "vcregress check-world" which is
> more in sync with it's peer "make check-world" instead of "vcregress
> all-taptest". I'm not sure whether we can also have "vcregress
> installcheck-world" as well.

check-world, if it spins new instances for each contrib/ test, would
be infinitely slower than installcheck-world.  I recall that's why
Andrew has been doing an installcheck for contribcheck to minimize the
load.  If you run the TAP tests, perhaps you don't really care anyway,
but I think that there is a case for making this set of targets run as
fast as we can, if we can, when TAP is disabled.

> Having said that, with these new options, are we going to have only below?
>
> vcregress check
> vcregress installcheck
> vcregress check-world
> vcregress installcheck-world (?)
>
> And remove others?

My take is that there is value for both installcheck-world and
check-world, depending on if we want to test on an installed instance
or not.  For CIs, check-world makes things simpler perhaps?

> vcregress plcheck
> vcregress contribcheck
> vcregress modulescheck
> vcregress ecpgcheck
> vcregress isolationcheck
> vcregress bincheck
> vcregress recoverycheck
> vcregress upgradecheck

I don't really see why we should do that, the code paths are the same
and the sub-routines would still be around, but don't cost much in
maintenance.
--
Michael

On Wed, Oct 6, 2021 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
> My take is that there is value for both installcheck-world and
> check-world, depending on if we want to test on an installed instance
> or not.  For CIs, check-world makes things simpler perhaps?
>
> > vcregress plcheck
> > vcregress contribcheck
> > vcregress modulescheck
> > vcregress ecpgcheck
> > vcregress isolationcheck
> > vcregress bincheck
> > vcregress recoverycheck
> > vcregress upgradecheck
>
> I don't really see why we should do that, the code paths are the same
> and the sub-routines would still be around, but don't cost much in
> maintenance.

Yeah, they can also be useful if someone wants to run tests
selectively. I'm just thinking that the "vcregress subscriptioncheck"
as proposed in my first email in this thread will also be useful (?)
along with the "vcregress check-world" and "vcregress
installcheck-world". Thoughts?

Regards,
Bharath Rupireddy.

On Wed, Oct 6, 2021 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 06, 2021 at 07:19:04AM +0530, Bharath Rupireddy wrote:
> > I was thinking of the same. +1 for "vcregress check-world" which is
> > more in sync with it's peer "make check-world" instead of "vcregress
> > all-taptest". I'm not sure whether we can also have "vcregress
> > installcheck-world" as well.
>
> check-world, if it spins new instances for each contrib/ test, would
> be infinitely slower than installcheck-world.  I recall that's why
> Andrew has been doing an installcheck for contribcheck to minimize the
> load.  If you run the TAP tests, perhaps you don't really care anyway,
> but I think that there is a case for making this set of targets run as
> fast as we can, if we can, when TAP is disabled.

Out of all the regression tests available with vcregress command
today, the tests shown at [1] require an already running postgres
instance (much like the installcheck). Should we change these for
"vcregress checkworld" so that they spin up tmp instances and run? I
don't want to go in this direction and change the code a lot.

To be simple, we could just have "vcregress installcheckworld" which
requires users to spin up an instance so that the tests shown at [1]
would run along with other tests [2] that spins up their own instance.
The problem with this approach is that user might setup a different
GUC value in the instance that he/she spins up expecting it to be
effective for the tests at [2] as well. I'm not sure if anyone would
do that. We can ignore "vcregress checkworld" but mention why we don't
do this in the documentation "something like it makes tests slower as
it spinus up lot of temporary pg instances".

Another idea, simplest of all, is that just have "vcregress
subscriptioncheck" as proposed in this first mail and not have
""vcregress checkworld" or "vcregress installcheckworld". With this
new option and the existing options of vcregress tool, it sort of
covers all the tests - core, TAP, contrib, bin, isolation, modules,
upgrade, recovery etc.

Thoughts?

[1]
vcregress installcheck
vcregress plcheck
vcregress contribcheck
vcregress modulescheck
vcregress isolationcheck

[2]
vcregress check
vcregress ecpgcheck
vcregress bincheck
vcregress recoverycheck
vcregress upgradecheck
vcregress subscriptioncheck

Regards,
Bharath Rupireddy.

On Wed, Oct 6, 2021 at 4:31 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Oct 6, 2021 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Oct 06, 2021 at 07:19:04AM +0530, Bharath Rupireddy wrote:
> > > I was thinking of the same. +1 for "vcregress check-world" which is
> > > more in sync with it's peer "make check-world" instead of "vcregress
> > > all-taptest". I'm not sure whether we can also have "vcregress
> > > installcheck-world" as well.
> >
> > check-world, if it spins new instances for each contrib/ test, would
> > be infinitely slower than installcheck-world.  I recall that's why
> > Andrew has been doing an installcheck for contribcheck to minimize the
> > load.  If you run the TAP tests, perhaps you don't really care anyway,
> > but I think that there is a case for making this set of targets run as
> > fast as we can, if we can, when TAP is disabled.
>
> Out of all the regression tests available with vcregress command
> today, the tests shown at [1] require an already running postgres
> instance (much like the installcheck). Should we change these for
> "vcregress checkworld" so that they spin up tmp instances and run? I
> don't want to go in this direction and change the code a lot.
>
> To be simple, we could just have "vcregress installcheckworld" which
> requires users to spin up an instance so that the tests shown at [1]
> would run along with other tests [2] that spins up their own instance.
> The problem with this approach is that user might setup a different
> GUC value in the instance that he/she spins up expecting it to be
> effective for the tests at [2] as well. I'm not sure if anyone would
> do that. We can ignore "vcregress checkworld" but mention why we don't
> do this in the documentation "something like it makes tests slower as
> it spinus up lot of temporary pg instances".
>
> Another idea, simplest of all, is that just have "vcregress
> subscriptioncheck" as proposed in this first mail and not have
> ""vcregress checkworld" or "vcregress installcheckworld". With this
> new option and the existing options of vcregress tool, it sort of
> covers all the tests - core, TAP, contrib, bin, isolation, modules,
> upgrade, recovery etc.
>
> Thoughts?
>
> [1]
> vcregress installcheck
> vcregress plcheck
> vcregress contribcheck
> vcregress modulescheck
> vcregress isolationcheck
>
> [2]
> vcregress check
> vcregress ecpgcheck
> vcregress bincheck
> vcregress recoverycheck
> vcregress upgradecheck
> vcregress subscriptioncheck

The problems with having "vcregress checkworld" are: 1) required code
modifications are more as the available "vcregress" test functions,
which required pre-started pg instance, can't be used directly. 2) it
looks like spinning up a separate postgres instance for all module
tests takes time on Windows which might make the test time longer. If
we were to have "vcregress installcheckworld", we might have to add
new code for converting some of the existing functions to not use a
pre-started pg instance.

IMHO, we can just have "vcregress subscriptioncheck" and let users
decide which tests they want to run.

I would like to hear more opinions on this.

Regards,
Bharath Rupireddy.

On 10/16/21 7:21 AM, Bharath Rupireddy wrote:
> On Wed, Oct 6, 2021 at 4:31 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> On Wed, Oct 6, 2021 at 7:52 AM Michael Paquier <michael@paquier.xyz> wrote:
>>> On Wed, Oct 06, 2021 at 07:19:04AM +0530, Bharath Rupireddy wrote:
>>>> I was thinking of the same. +1 for "vcregress check-world" which is
>>>> more in sync with it's peer "make check-world" instead of "vcregress
>>>> all-taptest". I'm not sure whether we can also have "vcregress
>>>> installcheck-world" as well.
>>> check-world, if it spins new instances for each contrib/ test, would
>>> be infinitely slower than installcheck-world.  I recall that's why
>>> Andrew has been doing an installcheck for contribcheck to minimize the
>>> load.  If you run the TAP tests, perhaps you don't really care anyway,
>>> but I think that there is a case for making this set of targets run as
>>> fast as we can, if we can, when TAP is disabled.
>> Out of all the regression tests available with vcregress command
>> today, the tests shown at [1] require an already running postgres
>> instance (much like the installcheck). Should we change these for
>> "vcregress checkworld" so that they spin up tmp instances and run? I
>> don't want to go in this direction and change the code a lot.
>>
>> To be simple, we could just have "vcregress installcheckworld" which
>> requires users to spin up an instance so that the tests shown at [1]
>> would run along with other tests [2] that spins up their own instance.
>> The problem with this approach is that user might setup a different
>> GUC value in the instance that he/she spins up expecting it to be
>> effective for the tests at [2] as well. I'm not sure if anyone would
>> do that. We can ignore "vcregress checkworld" but mention why we don't
>> do this in the documentation "something like it makes tests slower as
>> it spinus up lot of temporary pg instances".
>>
>> Another idea, simplest of all, is that just have "vcregress
>> subscriptioncheck" as proposed in this first mail and not have
>> ""vcregress checkworld" or "vcregress installcheckworld". With this
>> new option and the existing options of vcregress tool, it sort of
>> covers all the tests - core, TAP, contrib, bin, isolation, modules,
>> upgrade, recovery etc.
>>
>> Thoughts?
>>
>> [1]
>> vcregress installcheck
>> vcregress plcheck
>> vcregress contribcheck
>> vcregress modulescheck
>> vcregress isolationcheck
>>
>> [2]
>> vcregress check
>> vcregress ecpgcheck
>> vcregress bincheck
>> vcregress recoverycheck
>> vcregress upgradecheck
>> vcregress subscriptioncheck
> The problems with having "vcregress checkworld" are: 1) required code
> modifications are more as the available "vcregress" test functions,
> which required pre-started pg instance, can't be used directly. 2) it
> looks like spinning up a separate postgres instance for all module
> tests takes time on Windows which might make the test time longer. If
> we were to have "vcregress installcheckworld", we might have to add
> new code for converting some of the existing functions to not use a
> pre-started pg instance.
>
> IMHO, we can just have "vcregress subscriptioncheck" and let users
> decide which tests they want to run.
>
> I would like to hear more opinions on this.
>

My opinion hasn't changed. There is already a way to spell this and I'm
opposed to adding more such specific tests to vcregress.pl. Every such
addition we make increases the maintenance burden.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, Oct 16, 2021 at 6:35 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>
> > The problems with having "vcregress checkworld" are: 1) required code
> > modifications are more as the available "vcregress" test functions,
> > which required pre-started pg instance, can't be used directly. 2) it
> > looks like spinning up a separate postgres instance for all module
> > tests takes time on Windows which might make the test time longer. If
> > we were to have "vcregress installcheckworld", we might have to add
> > new code for converting some of the existing functions to not use a
> > pre-started pg instance.
> >
> > IMHO, we can just have "vcregress subscriptioncheck" and let users
> > decide which tests they want to run.
> >
> > I would like to hear more opinions on this.
> >
>
> My opinion hasn't changed. There is already a way to spell this and I'm
> opposed to adding more such specific tests to vcregress.pl. Every such
> addition we make increases the maintenance burden.

Thanks for your opinion. IIUC, the subscription tests can be run with
setting environment variables PROVE_FLAGS, PROVE_TESTS and the
"vcregress taptest" command right? I failed to set the environment
variables appropriately and couldn't run. Can you please let me know
the right way to run the test?

If any test can be run with a set of environment flags and "vcregress
taptest" command, then in the first place, it doesn't make sense to
have recoverycheck, upgragecheck and so on. Another thing is that the
list of "vcregress" commands cover almost all the tests core, tap,
bin, isolation, contrib tests except, subscription tests. If we add
"vcregress subscrtptioncheck", the list of tests that can be run with
the "vcregress" command will be complete without having to depend on
the environment variables.

IMHO, we can have "vcregress subscriptioncheck" to make it complete
and easier for the user to run those tests. However, let's hear what
other hackers have to say about this.

Another thing I noticed is that we don't have any mention of
"vcregress taptest" command in the docs [1], if I read the docs
correctly. How about we have it along with a  sample example on how to
run a specific TAP tests with it in the docs?

[1] - https://www.postgresql.org/docs/current/install-windows-full.html

Regards,
Bharath Rupireddy.

On 10/18/21 1:41 AM, Bharath Rupireddy wrote:
> On Sat, Oct 16, 2021 at 6:35 PM Andrew Dunstan <andrew@dunslane.net> wrote:
>>> The problems with having "vcregress checkworld" are: 1) required code
>>> modifications are more as the available "vcregress" test functions,
>>> which required pre-started pg instance, can't be used directly. 2) it
>>> looks like spinning up a separate postgres instance for all module
>>> tests takes time on Windows which might make the test time longer. If
>>> we were to have "vcregress installcheckworld", we might have to add
>>> new code for converting some of the existing functions to not use a
>>> pre-started pg instance.
>>>
>>> IMHO, we can just have "vcregress subscriptioncheck" and let users
>>> decide which tests they want to run.
>>>
>>> I would like to hear more opinions on this.
>>>
>> My opinion hasn't changed. There is already a way to spell this and I'm
>> opposed to adding more such specific tests to vcregress.pl. Every such
>> addition we make increases the maintenance burden.
> Thanks for your opinion. IIUC, the subscription tests can be run with
> setting environment variables PROVE_FLAGS, PROVE_TESTS and the
> "vcregress taptest" command right? I failed to set the environment
> variables appropriately and couldn't run. Can you please let me know
> the right way to run the test?



No extra environment flags should be required for MSVC.


This should suffice:


    vcregress taptest src/test/subscription


If you want to set PROVE_FLAGS the simplest thing is just to set it in
the environment before the above invocation


>
> If any test can be run with a set of environment flags and "vcregress
> taptest" command, then in the first place, it doesn't make sense to
> have recoverycheck, upgragecheck and so on. Another thing is that the
> list of "vcregress" commands cover almost all the tests core, tap,
> bin, isolation, contrib tests except, subscription tests. If we add
> "vcregress subscrtptioncheck", the list of tests that can be run with
> the "vcregress" command will be complete without having to depend on
> the environment variables.


The reason we have some of those other tests is because we didn't start
with having a generic taptest command in vcregress.pl.  So they are
simply legacy code. But that is no reason for adding to them.


>
> IMHO, we can have "vcregress subscriptioncheck" to make it complete
> and easier for the user to run those tests. However, let's hear what
> other hackers have to say about this.



I really fail to see how the invocation above is in any sense
significantly more complicated.


>
> Another thing I noticed is that we don't have any mention of
> "vcregress taptest" command in the docs [1], if I read the docs
> correctly. How about we have it along with a  sample example on how to
> run a specific TAP tests with it in the docs?
>
> [1] - https://www.postgresql.org/docs/current/install-windows-full.html
>


Yes, that's probably something that should be remedied.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/18/21 1:41 AM, Bharath Rupireddy wrote:
>> Another thing I noticed is that we don't have any mention of
>> "vcregress taptest" command in the docs [1], if I read the docs
>> correctly. How about we have it along with a  sample example on how to
>> run a specific TAP tests with it in the docs?
>> 
>> [1] - https://www.postgresql.org/docs/current/install-windows-full.html

> Yes, that's probably something that should be remedied.

Why would that belong in the installation instructions?
Running the TAP tests is documented at

https://www.postgresql.org/docs/devel/regress-tap.html

and if we need some Windows-specific instructions, ISTM that's
where to add them.

            regards, tom lane

On 10/18/21 9:37 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 10/18/21 1:41 AM, Bharath Rupireddy wrote:
>>> Another thing I noticed is that we don't have any mention of
>>> "vcregress taptest" command in the docs [1], if I read the docs
>>> correctly. How about we have it along with a  sample example on how to
>>> run a specific TAP tests with it in the docs?
>>>
>>> [1] - https://www.postgresql.org/docs/current/install-windows-full.html
>> Yes, that's probably something that should be remedied.
> Why would that belong in the installation instructions?
> Running the TAP tests is documented at
>
> https://www.postgresql.org/docs/devel/regress-tap.html
>
> and if we need some Windows-specific instructions, ISTM that's
> where to add them.



Well, see
<https://www.postgresql.org/docs/current/install-windows-full.html#id-1.6.5.8.12>


Maybe we should move that section.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Oct 18, 2021 at 09:56:41AM -0400, Andrew Dunstan wrote:
> Well, see
> <https://www.postgresql.org/docs/current/install-windows-full.html#id-1.6.5.8.12>
>
> Maybe we should move that section.

As this is the part of the docs where we document the builds, it
looks indeed a bit confusing to have all the requirements for the
TAP tests there.  The section "Regression Tests" cannot be used for
the case of VS, and the section for TAP is independent of that so we
could use platform-dependent sub-sections.

Could it be better to move all the contents of "Running the Regression
Tests" from the Windows installation page to the section of
"Regression Tests" instead?  That would mean spreading the knowledge
of vcregress.pl to more than one place, though.
--
Michael

On Mon, Oct 18, 2021 at 6:19 PM Andrew Dunstan <andrew@dunslane.net> wrote:
> > Thanks for your opinion. IIUC, the subscription tests can be run with
> > setting environment variables PROVE_FLAGS, PROVE_TESTS and the
> > "vcregress taptest" command right? I failed to set the environment
> > variables appropriately and couldn't run. Can you please let me know
> > the right way to run the test?
>
> No extra environment flags should be required for MSVC.
>
> This should suffice:
>
>     vcregress taptest src/test/subscription

Wow! This is so simple that I imagined.

> If you want to set PROVE_FLAGS the simplest thing is just to set it in
> the environment before the above invocation

Okay.

> > If any test can be run with a set of environment flags and "vcregress
> > taptest" command, then in the first place, it doesn't make sense to
> > have recoverycheck, upgragecheck and so on. Another thing is that the
> > list of "vcregress" commands cover almost all the tests core, tap,
> > bin, isolation, contrib tests except, subscription tests. If we add
> > "vcregress subscrtptioncheck", the list of tests that can be run with
> > the "vcregress" command will be complete without having to depend on
> > the environment variables.
>
> The reason we have some of those other tests is because we didn't start
> with having a generic taptest command in vcregress.pl.  So they are
> simply legacy code. But that is no reason for adding to them.

I get it, thanks.

> > IMHO, we can have "vcregress subscriptioncheck" to make it complete
> > and easier for the user to run those tests. However, let's hear what
> > other hackers have to say about this.
>
> I really fail to see how the invocation above is in any sense
> significantly more complicated.

Yes, the command "vcregress taptest src/test/subscription" is simple enough.

> > Another thing I noticed is that we don't have any mention of
> > "vcregress taptest" command in the docs [1], if I read the docs
> > correctly. How about we have it along with a  sample example on how to
> > run a specific TAP tests with it in the docs?
> >
> > [1] - https://www.postgresql.org/docs/current/install-windows-full.html
>
> Yes, that's probably something that should be remedied.

Yes, I will prepare a patch for it.

Regards,
Bharath Rupireddy.

On Tue, Oct 19, 2021 at 6:16 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Oct 18, 2021 at 09:56:41AM -0400, Andrew Dunstan wrote:
> > Well, see
> > <https://www.postgresql.org/docs/current/install-windows-full.html#id-1.6.5.8.12>
> >
> > Maybe we should move that section.
>
> As this is the part of the docs where we document the builds, it
> looks indeed a bit confusing to have all the requirements for the
> TAP tests there.  The section "Regression Tests" cannot be used for
> the case of VS, and the section for TAP is independent of that so we
> could use platform-dependent sub-sections.
>
> Could it be better to move all the contents of "Running the Regression
> Tests" from the Windows installation page to the section of
> "Regression Tests" instead?  That would mean spreading the knowledge
> of vcregress.pl to more than one place, though.

IMO, it is better to add a note in the "Running the Tests" section at
[1] and a link to the windows specific section at [2]. This will keep
all the windows specific things at one place without any duplication
of vcregress.pl knowledge. Thoughts? If okay, I will send a patch.

[1] https://www.postgresql.org/docs/devel/regress-run.html
[2] https://www.postgresql.org/docs/devel/install-windows-full.html#id-1.6.5.8.12

Regards,
Bharath Rupireddy.

On Tue, Oct 19, 2021 at 11:49 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Oct 19, 2021 at 6:16 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, Oct 18, 2021 at 09:56:41AM -0400, Andrew Dunstan wrote:
> > > Well, see
> > > <https://www.postgresql.org/docs/current/install-windows-full.html#id-1.6.5.8.12>
> > >
> > > Maybe we should move that section.
> >
> > As this is the part of the docs where we document the builds, it
> > looks indeed a bit confusing to have all the requirements for the
> > TAP tests there.  The section "Regression Tests" cannot be used for
> > the case of VS, and the section for TAP is independent of that so we
> > could use platform-dependent sub-sections.
> >
> > Could it be better to move all the contents of "Running the Regression
> > Tests" from the Windows installation page to the section of
> > "Regression Tests" instead?  That would mean spreading the knowledge
> > of vcregress.pl to more than one place, though.
>
> IMO, it is better to add a note in the "Running the Tests" section at
> [1] and a link to the windows specific section at [2]. This will keep
> all the windows specific things at one place without any duplication
> of vcregress.pl knowledge. Thoughts? If okay, I will send a patch.
>
> [1] https://www.postgresql.org/docs/devel/regress-run.html
> [2] https://www.postgresql.org/docs/devel/install-windows-full.html#id-1.6.5.8.12

Here's the documentation v1 patch that I've come up with. Please review it.

Regards,
Bharath Rupireddy.

On Mon, Dec 13, 2021 at 5:09 AM Juan José Santamaría Flecha
<juanjo.santamaria@gmail.com> wrote:
>
>
> On Thu, Oct 21, 2021 at 7:21 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>>
>> Here's the documentation v1 patch that I've come up with. Please review it.
>>
> There's a typo:
> +   To run an arbitrary TAP test set, run <command>vcregress taptest</command>
> +   comamnd. For example, use the following command for running subcription TAP
> +   tests:
> s/comamnd/command/
>
> But also the wording, I like better what vcregress prints as help, so something like:
> +   You can use <command>vcregress taptest TEST_DIR</command> to run an
> +   arbitrary TAP test set, where TEST_DIR is a required argument pointing to
> +   the directory where the tests reside. For example, use the following
> +   command for running the subcription TAP tests:

Thanks for the comments. Above looks good to me, changed that way, PSA v2.

Regards,
Bharath Rupireddy.

On Fri, Feb 11, 2022 at 12:29 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Feb 10, 2022 at 10:21:08PM +0530, Bharath Rupireddy wrote:
> > Thanks for the comments. Above looks good to me, changed that way, PSA v2.
>
> I spy a typo: subcription

Thanks. Corrected in v3 attached.

The CF entry https://commitfest.postgresql.org/36/3354/ was closed
with "Returned with Feedback". I'm not sure why. If the patch is still
of interest, I will add a new one for tracking.

Regards,
Bharath Rupireddy.