Thread: Small TAP tests cleanup for Windows and unused modules
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. -- Daniel Gustafsson https://vmware.com/ [1] https://postgr.es/m/0ba775a2-8aa0-0d56-d780-69427cf6f33d@dunslane.net
Attachment
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 definitelyvalid 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], 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]: https://www.postgresql.org/message-id/0ba775a2-8aa0-0d56-d780-69427cf6f33d@dunslane.net
--
Michael
Attachment
> 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')
From 681897c17c125a0ef380aa3ef0cba786dba24d0b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 25 Mar 2022 12:36:34 +0000
Subject: [PATCH] Remove more unused module imports in test
Commit 7dac61402e34c6d41d5d11cdc4c6a55f91e24026 removed a bunch of
superflous `use Config;` and `use Cwd;` statements, but some were left
behind.
---
src/bin/pg_ctl/t/001_start_stop.pl | 3 ---
src/bin/pg_rewind/t/RewindTest.pm | 1 -
src/test/ldap/t/001_auth.pl | 1 -
3 files changed, 5 deletions(-)
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl b/src/bin/pg_ctl/t/001_start_stop.pl
index 3b45390ced..fdffd76d99 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -4,9 +4,6 @@
use strict;
use warnings;
-use Config;
-use Fcntl ':mode';
-use File::stat qw{lstat};
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 5651602858..1e34768e27 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -35,7 +35,6 @@ package RewindTest;
use warnings;
use Carp;
-use Config;
use Exporter 'import';
use File::Copy;
use File::Path qw(rmtree);
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 9f15248935..b342146e55 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -6,7 +6,6 @@
use PostgreSQL::Test::Utils;
use PostgreSQL::Test::Cluster;
use Test::More;
-use Config;
my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
--
2.30.2
> 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/