Small TAP tests cleanup for Windows and unused modules

On 2/16/22 16:36, Daniel Gustafsson wrote:

Seeing msys in TAP tests mentioned in a thread [1] tonight reminded me about
two related (well, one of them) patches I had sitting around, so rather than
forgetting again here are some small cleanups.

0001 attempts to streamline how we detect Windows in the TAP tests (after that
there is a single msys check left that I'm not sure about, but [1] seems to
imply it could go); 0002 removes some unused module includes which either were
used at some point in the past or likely came from copy/paste.

0002 looks OK at first glance.

0001 is something we should investigate. It's really going in the wrong
direction I suspect. We should be looking to narrow the scope of these
platform-specific bits of processing, not expand them.

cheers

andrew

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

On 16 Feb 2022, at 23:04, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2/16/22 16:36, Daniel Gustafsson wrote:

Seeing msys in TAP tests mentioned in a thread [1] tonight reminded me about
two related (well, one of them) patches I had sitting around, so rather than
forgetting again here are some small cleanups.

0001 attempts to streamline how we detect Windows in the TAP tests (after that
there is a single msys check left that I'm not sure about, but [1] seems to
imply it could go); 0002 removes some unused module includes which either were
used at some point in the past or likely came from copy/paste.

0002 looks OK at first glance.

Thanks!

0001 is something we should investigate. It's really going in the wrong
direction I suspect. We should be looking to narrow the scope of these
platform-specific bits of processing, not expand them.

No arguments there, I was only looking at making sure that we detect the OS in
the same way everywhere, but if you're working towards eliminating this there
is little use in changing anything.

--
Daniel Gustafsson https://vmware.com/

On 2/16/22 17:04, Andrew Dunstan wrote:

On 2/16/22 16:36, Daniel Gustafsson wrote:

Seeing msys in TAP tests mentioned in a thread [1] tonight reminded me about
two related (well, one of them) patches I had sitting around, so rather than
forgetting again here are some small cleanups.

0001 attempts to streamline how we detect Windows in the TAP tests (after that
there is a single msys check left that I'm not sure about, but [1] seems to
imply it could go); 0002 removes some unused module includes which either were
used at some point in the past or likely came from copy/paste.

0002 looks OK at first glance.

0001 is something we should investigate. It's really going in the wrong
direction I suspect. We should be looking to narrow the scope of these
platform-specific bits of processing, not expand them.

More specifically, all the tests in question are now passing on jacana
and fairywren, and their $Config{osname} is NOT 'msys' (it's 'MSWin32').

So we should simply remove any line that ends "if $Config{osname} eq
'msys';" since we don't have any such animals any more.

cheers

andrew

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

Hi,

On February 16, 2022 2:04:01 PM PST, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2/16/22 16:36, Daniel Gustafsson wrote:

Seeing msys in TAP tests mentioned in a thread [1] tonight reminded me about
two related (well, one of them) patches I had sitting around, so rather than
forgetting again here are some small cleanups.

0001 attempts to streamline how we detect Windows in the TAP tests (after that
there is a single msys check left that I'm not sure about, but [1] seems to
imply it could go); 0002 removes some unused module includes which either were
used at some point in the past or likely came from copy/paste.

0002 looks OK at first glance.

0001 is something we should investigate. It's really going in the wrong
direction I suspect. We should be looking to narrow the scope of these
platform-specific bits of processing, not expand them.

Yes. The vast majority of those skips looks frankly irresponsible, indicating bugs in the test or postgres. There's definitely valid cases (e.g. testing sysv shm), but most don't look valid to me.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Feb 16, 2022 at 05:36:14PM -0500, Andrew Dunstan wrote:

More specifically, all the tests in question are now passing on jacana
and fairywren, and their $Config{osname} is NOT 'msys' (it's 'MSWin32').

Indeed. 0001 is incorrect.

So we should simply remove any line that ends "if $Config{osname} eq
'msys';" since we don't have any such animals any more.

So, that's related to [1]/messages/by-id/0ba775a2-8aa0-0d56-d780-69427cf6f33d@dunslane.net -- Michael, meaning that we don't have any buildfarm
members that run the perl flavor from Msys. And, as a result, we
don't need to worry about the CRLF conversions in any of the perl
modules? That would be nice.

[1]: /messages/by-id/0ba775a2-8aa0-0d56-d780-69427cf6f33d@dunslane.net -- Michael
--
Michael

On 16 Feb 2022, at 23:36, Andrew Dunstan <andrew@dunslane.net> wrote:
On 2/16/22 17:04, Andrew Dunstan wrote:

On 2/16/22 16:36, Daniel Gustafsson wrote:

Seeing msys in TAP tests mentioned in a thread [1] tonight reminded me about
two related (well, one of them) patches I had sitting around, so rather than
forgetting again here are some small cleanups.

0001 attempts to streamline how we detect Windows in the TAP tests (after that
there is a single msys check left that I'm not sure about, but [1] seems to
imply it could go); 0002 removes some unused module includes which either were
used at some point in the past or likely came from copy/paste.

0002 looks OK at first glance.

0001 is something we should investigate. It's really going in the wrong
direction I suspect. We should be looking to narrow the scope of these
platform-specific bits of processing, not expand them.

More specifically, all the tests in question are now passing on jacana
and fairywren, and their $Config{osname} is NOT 'msys' (it's 'MSWin32').

+1

So we should simply remove any line that ends "if $Config{osname} eq
'msys';" since we don't have any such animals any more.

Since msys is discussed in the other thread let's drop the 0001 from this
thread and just go ahead with 0002.

--
Daniel Gustafsson https://vmware.com/

On 18 Feb 2022, at 22:02, Daniel Gustafsson <daniel@yesql.se> wrote:

.. let's drop the 0001 from this thread and just go ahead with 0002.

I applied the 0002 patch today, cleaning up the unused module imports.

--
Daniel Gustafsson https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:

On 18 Feb 2022, at 22:02, Daniel Gustafsson <daniel@yesql.se> wrote:

.. let's drop the 0001 from this thread and just go ahead with 0002.

I applied the 0002 patch today, cleaning up the unused module imports.

A quick git grep¹ revealed a few more extraneous `use Config;`
statements (and manual inspection a few more unused modules in one
file). Here's a patch that removes those. It passes tests using

./configure --enable-tap-tests --with-ldap
make check-world PG_TEST_EXTRA=ldap

- ilmari

[1]: git grep -L '[%@$]Config\b' $(git grep -l 'use Config')

On 25 Mar 2022, at 13:42, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:

A quick git grep¹ revealed a few more extraneous `use Config;`
statements (and manual inspection a few more unused modules in one
file).

Thanks for digging these up, they look correct to me.

src/bin/pg_ctl/t/001_start_stop.pl | 3 ---
src/bin/pg_rewind/t/RewindTest.pm | 1 -

These Config references were removed in 1c6d46293. Fcntl ':mode' and
File::stat were added in c37b3d08c which was probably a leftover from an
earlier version of the patch, as the function using these was added to
PostgresNode in that commit.

src/test/ldap/t/001_auth.pl | 1 -

The Config reference here was added in ee56c3b21 which in turn use $^O instead
of interrogating Config, so it can be removed as well.

I'll take it for a tour on the CI and will then apply.

--
Daniel Gustafsson https://vmware.com/

On 27 Mar 2022, at 00:20, Daniel Gustafsson <daniel@yesql.se> wrote:

I'll take it for a tour on the CI and will then apply.

Which is now done, thanks!

--
Daniel Gustafsson https://vmware.com/