Thread: Create shorthand for including all extra tests

Create shorthand for including all extra tests

From
Nazir Bilal Yavuz
Date:
Hi,

I realized that I forgot to add the new extra test to my test scripts.
So, I thought maybe we can use shorthand for including all extra
tests. With that, running a full testsuite is easier without having to
keep up with new tests and updates.

I created an 'all' option for PG_TEST_EXTRA to enable all test suites
defined under PG_TEST_EXTRA. I created the check_extra_tests_enabled()
function in the Test/Utils.pm file. This function takes the test's
name as an input and checks if PG_TEST_EXTRA contains 'all' or this
test's name.

I thought another advantage could be that this can be used in CI. But
when 'wal_consistency_checking' is enabled, CI times get longer since
it does resource intensive operations.

Any kind of feedback would be appreciated.

Regards,
Nazir Bilal Yavuz
Microsoft

Attachment

Re: Create shorthand for including all extra tests

From
Tom Lane
Date:
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> I created an 'all' option for PG_TEST_EXTRA to enable all test suites
> defined under PG_TEST_EXTRA.

I think this is a seriously bad idea.  The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.

            regards, tom lane



Re: Create shorthand for including all extra tests

From
Daniel Gustafsson
Date:
> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
>> I created an 'all' option for PG_TEST_EXTRA to enable all test suites
>> defined under PG_TEST_EXTRA.
>
> I think this is a seriously bad idea.  The entire point of not including
> certain tests in check-world by default is that the omitted tests are
> security hazards, so a developer or buildfarm owner should review each
> one before deciding whether to activate it on their machine.

I dunno, I've certainly managed to not run the tests I hoped to multiple times.
I think it could be useful for sandboxed testrunners which are destroyed after
each run. There is for sure a foot-gun angle to it, no question about that.

--
Daniel Gustafsson




Re: Create shorthand for including all extra tests

From
Noah Misch
Date:
On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:
> > On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> >> I created an 'all' option for PG_TEST_EXTRA to enable all test suites
> >> defined under PG_TEST_EXTRA.
> > 
> > I think this is a seriously bad idea.  The entire point of not including
> > certain tests in check-world by default is that the omitted tests are
> > security hazards, so a developer or buildfarm owner should review each
> > one before deciding whether to activate it on their machine.
> 
> I dunno, I've certainly managed to not run the tests I hoped to multiple times.
> I think it could be useful for sandboxed testrunners which are destroyed after
> each run. There is for sure a foot-gun angle to it, no question about that.

Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
they treat the loopback interface as private, so anyone with access to
loopback interface ports can hijack the test.  I'd be fine with e.g.
PG_TEST_EXTRA=private-lo activating all of those.  We don't gain by inviting
the tester to review the tests to rediscover this common factor.



Re: Create shorthand for including all extra tests

From
Tom Lane
Date:
Noah Misch <noah@leadboat.com> writes:
> On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:
>> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think this is a seriously bad idea.  The entire point of not including
>>> certain tests in check-world by default is that the omitted tests are
>>> security hazards, so a developer or buildfarm owner should review each
>>> one before deciding whether to activate it on their machine.

> Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
> they treat the loopback interface as private, so anyone with access to
> loopback interface ports can hijack the test.  I'd be fine with e.g.
> PG_TEST_EXTRA=private-lo activating all of those.  We don't gain by inviting
> the tester to review the tests to rediscover this common factor.

Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though.  Maybe we could invent
categories that can be used in place of individual test names?
For now,

    PG_TEST_EXTRA="needs-private-lo slow"

would cover the territory of "all", and I think it'd be very seldom
that we'd have to invent new categories here (though maybe I lack
imagination today).

            regards, tom lane



Re: Create shorthand for including all extra tests

From
Noah Misch
Date:
On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:
> >> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> I think this is a seriously bad idea.  The entire point of not including
> >>> certain tests in check-world by default is that the omitted tests are
> >>> security hazards, so a developer or buildfarm owner should review each
> >>> one before deciding whether to activate it on their machine.
> 
> > Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
> > they treat the loopback interface as private, so anyone with access to
> > loopback interface ports can hijack the test.  I'd be fine with e.g.
> > PG_TEST_EXTRA=private-lo activating all of those.  We don't gain by inviting
> > the tester to review the tests to rediscover this common factor.
> 
> Yeah, I could live with something like that from the security standpoint.
> Not sure if it helps Nazir's use-case though.  Maybe we could invent
> categories that can be used in place of individual test names?
> For now,
> 
>     PG_TEST_EXTRA="needs-private-lo slow"
> 
> would cover the territory of "all", and I think it'd be very seldom
> that we'd have to invent new categories here (though maybe I lack
> imagination today).

I could imagine categories for filesystem bytes and RAM bytes.  Also, while
needs-private-lo has a bounded definition, "slow" doesn't.  If today's one
"slow" test increases check-world duration by 1.1x, we may not let a
100x-increase test use the same keyword.

If one introduced needs-private-lo, the present spelling of "all" would be
"needs-private-lo wal_consistency_checking".  Looks okay to me.  Doing nothing
here wouldn't be ruinous, of course.



Re: Create shorthand for including all extra tests

From
Nazir Bilal Yavuz
Date:
Hi,

Thanks for the feedback! I updated the patch, 'needs-private-lo'
option enables kerberos, ldap, load_balance and ssl extra tests now.

> On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote:
> > Yeah, I could live with something like that from the security standpoint.
> > Not sure if it helps Nazir's use-case though.  Maybe we could invent
> > categories that can be used in place of individual test names?
> > For now,

Yes, that is not ideal for my use-case but still better.

On Tue, 5 Sept 2023 at 00:09, Noah Misch <noah@leadboat.com> wrote:
>
> I could imagine categories for filesystem bytes and RAM bytes.  Also, while
> needs-private-lo has a bounded definition, "slow" doesn't.  If today's one
> "slow" test increases check-world duration by 1.1x, we may not let a
> 100x-increase test use the same keyword.

I agree. I didn't create a new category as 'slow' but still open to suggestions.

I am not very familiar with perl syntax, I would like to hear your
opinions on how the implementation of the check_extra_text_enabled()
function could be done better.

Regards,
Nazir Bilal Yavuz
Microsoft

Attachment

Re: Create shorthand for including all extra tests

From
Daniel Gustafsson
Date:
> On 4 Sep 2023, at 23:09, Noah Misch <noah@leadboat.com> wrote:

> I could imagine categories for filesystem bytes and RAM bytes.  Also, while
> needs-private-lo has a bounded definition, "slow" doesn't.  If today's one
> "slow" test increases check-world duration by 1.1x, we may not let a
> 100x-increase test use the same keyword.

Agreed, the names should be descriptive enough to contain a boundary.  Any new
test which is orders of magnitude slower than an existing test suite most
likely will have one/more boundary characteristics not shared with existing
suites.  The test in 20210423204306.5osfpkt2ggaedyvy@alap3.anarazel.de for
autovacuum wraparound comes to mind as one that would warrant a new category.

> If one introduced needs-private-lo, the present spelling of "all" would be
> "needs-private-lo wal_consistency_checking".

I think it makes sense to invent a new PG_TEST_EXTRA category which (for now)
only contains wal_consistency_checking to make it consistent, such that "all"
can be achieved by a set of categories.

--
Daniel Gustafsson




Re: Create shorthand for including all extra tests

From
Peter Eisentraut
Date:
On 04.09.23 22:30, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
>> On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:
>>> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I think this is a seriously bad idea.  The entire point of not including
>>>> certain tests in check-world by default is that the omitted tests are
>>>> security hazards, so a developer or buildfarm owner should review each
>>>> one before deciding whether to activate it on their machine.
> 
>> Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
>> they treat the loopback interface as private, so anyone with access to
>> loopback interface ports can hijack the test.  I'd be fine with e.g.
>> PG_TEST_EXTRA=private-lo activating all of those.  We don't gain by inviting
>> the tester to review the tests to rediscover this common factor.
> 
> Yeah, I could live with something like that from the security standpoint.
> Not sure if it helps Nazir's use-case though.  Maybe we could invent
> categories that can be used in place of individual test names?
> For now,
> 
>     PG_TEST_EXTRA="needs-private-lo slow"
> 
> would cover the territory of "all", and I think it'd be very seldom
> that we'd have to invent new categories here (though maybe I lack
> imagination today).

At least the kerberos tests also appear to require a lot of randomness 
for their setup, and sometimes in VM environments they hang for minutes 
until they get that.  I suppose that would go under "slow".

Also, at least in my mind, when we added the kerberos and ldap tests, a 
partial reason for excluding them from the default run was "requires 
additional unusual software to be installed".  The additional kerberos 
and ldap server software used in those tests is not covered by 
configure/meson, so it's a bit more DIY.





Re: Create shorthand for including all extra tests

From
Peter Eisentraut
Date:
On 05.09.23 19:26, Nazir Bilal Yavuz wrote:
> Thanks for the feedback! I updated the patch, 'needs-private-lo'
> option enables kerberos, ldap, load_balance and ssl extra tests now.

As was discussed, I don't think "needs private lo" is the only condition 
for these tests.  At least kerberos and ldap also need extra software 
installed, and load_balance might need editing the system's hosts file. 
So someone would still need to familiarize themselves with these tests 
individually before setting a global option like this.

Also, if we were to create test groupings like this, I think the 
implementation should be different.  The way you have it, there is a 
sort of central registry of all affected tests in 
src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. 
  I would prefer a more decentralized approach where each test decides 
on its own whether to run, with pseudo-code conditionals like

if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains 
"needs-private-lo"))
   skip_all

Anyway, at the moment, I don't see a sensible way to group these things 
beyond what we have now (effectively, "ldap" is already a group, because 
it affects more than one test suite).  Right now, we have six possible 
values, which is probably just about doable to keep track of manually. 
If we get a lot more, then we need to look into this again, but maybe 
then we'll also have more patterns to group things around.




Re: Create shorthand for including all extra tests

From
Nazir Bilal Yavuz
Date:
Hi,

On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 05.09.23 19:26, Nazir Bilal Yavuz wrote:
> > Thanks for the feedback! I updated the patch, 'needs-private-lo'
> > option enables kerberos, ldap, load_balance and ssl extra tests now.
>
> As was discussed, I don't think "needs private lo" is the only condition
> for these tests.  At least kerberos and ldap also need extra software
> installed, and load_balance might need editing the system's hosts file.
> So someone would still need to familiarize themselves with these tests
> individually before setting a global option like this.
>
> Also, if we were to create test groupings like this, I think the
> implementation should be different.  The way you have it, there is a
> sort of central registry of all affected tests in
> src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests.
>   I would prefer a more decentralized approach where each test decides
> on its own whether to run, with pseudo-code conditionals like
>
> if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains
> "needs-private-lo"))
>    skip_all
>
> Anyway, at the moment, I don't see a sensible way to group these things
> beyond what we have now (effectively, "ldap" is already a group, because
> it affects more than one test suite).  Right now, we have six possible
> values, which is probably just about doable to keep track of manually.
> If we get a lot more, then we need to look into this again, but maybe
> then we'll also have more patterns to group things around.

I see your point. It looks like the best option is to reevaluate this
if there are more PG_TEST_EXTRA options.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft



Re: Create shorthand for including all extra tests

From
Peter Eisentraut
Date:
On 15.01.24 09:54, Nazir Bilal Yavuz wrote:
> Hi,
> 
> On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 05.09.23 19:26, Nazir Bilal Yavuz wrote:
>>> Thanks for the feedback! I updated the patch, 'needs-private-lo'
>>> option enables kerberos, ldap, load_balance and ssl extra tests now.
>>
>> As was discussed, I don't think "needs private lo" is the only condition
>> for these tests.  At least kerberos and ldap also need extra software
>> installed, and load_balance might need editing the system's hosts file.
>> So someone would still need to familiarize themselves with these tests
>> individually before setting a global option like this.
>>
>> Also, if we were to create test groupings like this, I think the
>> implementation should be different.  The way you have it, there is a
>> sort of central registry of all affected tests in
>> src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests.
>>    I would prefer a more decentralized approach where each test decides
>> on its own whether to run, with pseudo-code conditionals like
>>
>> if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains
>> "needs-private-lo"))
>>     skip_all
>>
>> Anyway, at the moment, I don't see a sensible way to group these things
>> beyond what we have now (effectively, "ldap" is already a group, because
>> it affects more than one test suite).  Right now, we have six possible
>> values, which is probably just about doable to keep track of manually.
>> If we get a lot more, then we need to look into this again, but maybe
>> then we'll also have more patterns to group things around.
> 
> I see your point. It looks like the best option is to reevaluate this
> if there are more PG_TEST_EXTRA options.

Ok, I'm closing this commitfest entry.