Clean up some pg_dump tests

Started by Peter Eisentrautover 2 years ago5 messages
#1Peter Eisentraut
peter@eisentraut.org
1 attachment(s)

Following [0]/messages/by-id/3ddf79f2-8b7b-a093-11d2-5c739bc64f86@eisentraut.org, I did a broader analysis of some dubious or nonsensical
like/unlike combinations in the pg_dump tests.

This includes

1) Remove useless entries from "unlike" lists. Runs that are not
listed in "like" don't need to be excluded in "unlike".

2) Ensure there is always a "like" list, even if it is empty. This
makes the test more self-documenting.

3) Use predefined lists such as %full_runs where appropriate, instead
of listing all runs separately.

I also added code that checks 1 and 2 automatically and issues a message
for violations. (This is currently done with "diag". We could also
make it an error.)

The results are in the attached patch.

[0]: /messages/by-id/3ddf79f2-8b7b-a093-11d2-5c739bc64f86@eisentraut.org
/messages/by-id/3ddf79f2-8b7b-a093-11d2-5c739bc64f86@eisentraut.org

Attachments:

0001-Clean-up-some-pg_dump-tests.patchtext/plain; charset=UTF-8; name=0001-Clean-up-some-pg_dump-tests.patchDownload
From bf13888e63431c513e440982a72a76d1d7193777 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 2 Oct 2023 08:58:23 +0200
Subject: [PATCH] Clean up some pg_dump tests

1) Remove useless entries from "unlike" lists.  Runs that are not
   listed in "like" don't need to be excluded in "unlike".

2) Ensure there is always a "like" list, even if it is empty.  This
   makes the test more self-documenting.

3) Use predefined lists such as %full_runs where appropriate, instead
   of listing all runs separately.

Also add code that checks 1 and 2 automatically and issues a message
for violations.
---
 src/bin/pg_dump/t/002_pg_dump.pl | 72 +++++++-------------------------
 1 file changed, 15 insertions(+), 57 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 55e98ec8e3..8a93910269 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -818,7 +818,7 @@
 		regexp => qr/^\QALTER COLLATION public.test0 OWNER TO \E.+;/m,
 		collation => 1,
 		like => { %full_runs, section_pre_data => 1, },
-		unlike => { %dump_test_schema_runs, no_owner => 1, },
+		unlike => { no_owner => 1, },
 	},
 
 	'ALTER FOREIGN DATA WRAPPER dummy OWNER TO' => {
@@ -977,7 +977,7 @@
 		create_sql =>
 		  'ALTER SCHEMA public OWNER TO "regress_quoted  \"" role";',
 		regexp => qr/^(GRANT|REVOKE)/m,
-		unlike => { defaults_public_owner => 1 },
+		like => {},
 	},
 
 	'ALTER SEQUENCE test_table_col1_seq' => {
@@ -1285,9 +1285,7 @@
 		  { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
 		unlike => {
 			exclude_dump_test_schema => 1,
-			only_dump_test_table => 1,
 			no_owner => 1,
-			role => 1,
 			only_dump_measurement => 1,
 		},
 	},
@@ -1351,7 +1349,6 @@
 			binary_upgrade => 1,
 			no_large_objects => 1,
 			schema_only => 1,
-			section_pre_data => 1,
 		},
 	},
 
@@ -3210,7 +3207,6 @@
 			binary_upgrade => 1,
 			exclude_dump_test_schema => 1,
 			schema_only => 1,
-			only_dump_measurement => 1,
 		},
 	},
 
@@ -3457,7 +3453,6 @@
 	'Disabled trigger on partition is not created' => {
 		regexp => qr/CREATE TRIGGER test_trigger.*ON dump_test_second_schema/,
 		like => {},
-		unlike => { %full_runs, %dump_test_schema_runs },
 	},
 
 	# Triggers on partitions should not be dropped individually
@@ -3834,35 +3829,12 @@
 		\QCREATE INDEX measurement_city_id_logdate_idx ON ONLY dump_test.measurement USING\E
 		/xm,
 		like => {
-			binary_upgrade => 1,
-			clean => 1,
-			clean_if_exists => 1,
-			compression => 1,
-			createdb => 1,
-			defaults => 1,
-			exclude_test_table => 1,
-			exclude_test_table_data => 1,
-			no_toast_compression => 1,
-			no_large_objects => 1,
-			no_privs => 1,
-			no_owner => 1,
-			no_table_access_method => 1,
-			only_dump_test_schema => 1,
-			pg_dumpall_dbprivs => 1,
-			pg_dumpall_exclude => 1,
-			schema_only => 1,
+			%full_runs,
+			%dump_test_schema_runs,
 			section_post_data => 1,
-			test_schema_plus_large_objects => 1,
-			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
 			exclude_dump_test_schema => 1,
-			only_dump_test_table => 1,
-			pg_dumpall_globals => 1,
-			pg_dumpall_globals_clean => 1,
-			role => 1,
-			section_pre_data => 1,
 			exclude_measurement => 1,
 		},
 	},
@@ -3913,7 +3885,6 @@
 			role => 1,
 			section_post_data => 1,
 			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
 			exclude_measurement => 1,
@@ -3927,35 +3898,12 @@
 		\QALTER INDEX dump_test.measurement_pkey ATTACH PARTITION dump_test_second_schema.measurement_y2006m2_pkey\E
 		/xm,
 		like => {
-			binary_upgrade => 1,
-			clean => 1,
-			clean_if_exists => 1,
-			compression => 1,
-			createdb => 1,
-			defaults => 1,
-			exclude_dump_test_schema => 1,
-			exclude_test_table => 1,
-			exclude_test_table_data => 1,
-			no_toast_compression => 1,
-			no_large_objects => 1,
-			no_privs => 1,
-			no_owner => 1,
-			no_table_access_method => 1,
-			pg_dumpall_dbprivs => 1,
-			pg_dumpall_exclude => 1,
+			%full_runs,
 			role => 1,
-			schema_only => 1,
 			section_post_data => 1,
 			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
-			only_dump_test_schema => 1,
-			only_dump_test_table => 1,
-			pg_dumpall_globals => 1,
-			pg_dumpall_globals_clean => 1,
-			section_pre_data => 1,
-			test_schema_plus_large_objects => 1,
 			exclude_measurement => 1,
 		},
 	},
@@ -4951,6 +4899,16 @@
 			next;
 		}
 
+		if (!defined($tests{$test}->{like}))
+		{
+			diag "missing like in test \"$test\"";
+		}
+		if ($tests{$test}->{unlike}->{$test_key} &&
+			!defined($tests{$test}->{like}->{$test_key}))
+		{
+			diag "useless unlike \"$test_key\" in test \"$test\"";
+		}
+
 		# Run the test listed as a like, unless it is specifically noted
 		# as an unlike (generally due to an explicit exclusion or similar).
 		if ($tests{$test}->{like}->{$test_key}

base-commit: c8ec5e0543b90372c8e6d5cc2cd3d2ff89ca0e82
-- 
2.42.0

#2Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Peter Eisentraut (#1)
Re: Clean up some pg_dump tests

I tried this out. I agree it's a good change. BTW, this made me
realize that "unlike" is not a good name: maybe it should be called
"except".

On 2023-Oct-02, Peter Eisentraut wrote:

+		if (!defined($tests{$test}->{like}))
+		{
+			diag "missing like in test \"$test\"";
+		}
+		if ($tests{$test}->{unlike}->{$test_key} &&
+			!defined($tests{$test}->{like}->{$test_key}))
+		{
+			diag "useless unlike \"$test_key\" in test \"$test\"";
+		}

I would add quotes to the words "like" and "unlike" there. Otherwise,
these sentences are hard to parse. Also, some commentary on what this
is about seems warranted: maybe "Check that this test properly defines
which dumps the output should match on." or similar.

I didn't like using diag(), because automated runs will not alert to any
problems. Now maybe that's not critical, but I fear that people would
not notice problems if they are just noise in the output. Let's make
them test errors. fail() seems good enough: with the lines I quote
above and omitting the test corrections, I get this, which seems good
enough:

# Failed test 'useless unlike "binary_upgrade" in test "Disabled trigger on partition is not created"'
# at t/002_pg_dump.pl line 4960.

# Failed test 'useless unlike "clean" in test "Disabled trigger on partition is not created"'
# at t/002_pg_dump.pl line 4960.

[... a few others ...]

Test Summary Report
-------------------
t/002_pg_dump.pl (Wstat: 15104 (exited 59) Tests: 11368 Failed: 59)
Failed tests: 241, 486, 731, 1224, 1473, 1719, 1968, 2217
2463, 2712, 2961, 3207, 3452, 3941, 4190
4442, 4692, 4735-4736, 4943, 5094, 5189
5242, 5341, 5436, 5681, 5926, 6171, 6660
6905, 7150, 7395, 7640, 7683, 7762, 7887
7930, 7941, 8134, 8187, 8229, 8287, 8626
8871, 8924, 9023, 9170, 9269, 9457, 9515
9704, 9762, 10345, 10886, 10985, 11105
11123, 11134, 11327
Non-zero exit status: 59
Files=5, Tests=11482, 15 wallclock secs ( 0.43 usr 0.04 sys + 4.56 cusr 1.63 csys = 6.66 CPU)
Result: FAIL

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ni aún el genio muy grande llegaría muy lejos
si tuviera que sacarlo todo de su propio interior" (Goethe)

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Alvaro Herrera (#2)
1 attachment(s)
Re: Clean up some pg_dump tests

On 09.10.23 11:20, Alvaro Herrera wrote:

I tried this out. I agree it's a good change. BTW, this made me
realize that "unlike" is not a good name: maybe it should be called
"except".

right

I would add quotes to the words "like" and "unlike" there. Otherwise,
these sentences are hard to parse. Also, some commentary on what this
is about seems warranted: maybe "Check that this test properly defines
which dumps the output should match on." or similar.

Done.

I also moved the code a bit earlier, before the checks for supported
compression libraries etc., so it runs even if those cause a skip.

I didn't like using diag(), because automated runs will not alert to any
problems. Now maybe that's not critical, but I fear that people would
not notice problems if they are just noise in the output. Let's make
them test errors. fail() seems good enough: with the lines I quote
above and omitting the test corrections, I get this, which seems good
enough:

After researching this a bit more, I think "die" is the convention for
problems in the test definitions themselves. (Otherwise, you're writing
a test about the tests, which would be a bit weird.) The result is
approximately the same.

Attachments:

v2-0001-Clean-up-some-pg_dump-tests.patchtext/plain; charset=UTF-8; name=v2-0001-Clean-up-some-pg_dump-tests.patchDownload
From ec4380986519f966303c14ea49223f0cf7729220 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 10 Oct 2023 09:54:43 +0200
Subject: [PATCH v2] Clean up some pg_dump tests

1) Remove useless entries from "unlike" lists.  Runs that are not
   listed in "like" don't need to be excluded in "unlike".

2) Ensure there is always a "like" list, even if it is empty.  This
   makes the test more self-documenting.

3) Use predefined lists such as %full_runs where appropriate, instead
   of listing all runs separately.

Also add code that checks 1 and 2 automatically and dies with an error
for violations.

Discussion: https://www.postgresql.org/message-id/flat/1f8cb371-e84e-434e-0367-6b716fb16fa1@eisentraut.org
---
 src/bin/pg_dump/t/002_pg_dump.pl | 78 +++++++++-----------------------
 1 file changed, 21 insertions(+), 57 deletions(-)

diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 326c9a7639..eb3ec534b4 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -818,7 +818,7 @@
 		regexp => qr/^\QALTER COLLATION public.test0 OWNER TO \E.+;/m,
 		collation => 1,
 		like => { %full_runs, section_pre_data => 1, },
-		unlike => { %dump_test_schema_runs, no_owner => 1, },
+		unlike => { no_owner => 1, },
 	},
 
 	'ALTER FOREIGN DATA WRAPPER dummy OWNER TO' => {
@@ -977,7 +977,7 @@
 		create_sql =>
 		  'ALTER SCHEMA public OWNER TO "regress_quoted  \"" role";',
 		regexp => qr/^(GRANT|REVOKE)/m,
-		unlike => { defaults_public_owner => 1 },
+		like => {},
 	},
 
 	'ALTER SEQUENCE test_table_col1_seq' => {
@@ -1285,9 +1285,7 @@
 		  { %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
 		unlike => {
 			exclude_dump_test_schema => 1,
-			only_dump_test_table => 1,
 			no_owner => 1,
-			role => 1,
 			only_dump_measurement => 1,
 		},
 	},
@@ -1351,7 +1349,6 @@
 			binary_upgrade => 1,
 			no_large_objects => 1,
 			schema_only => 1,
-			section_pre_data => 1,
 		},
 	},
 
@@ -3210,7 +3207,6 @@
 			binary_upgrade => 1,
 			exclude_dump_test_schema => 1,
 			schema_only => 1,
-			only_dump_measurement => 1,
 		},
 	},
 
@@ -3457,7 +3453,6 @@
 	'Disabled trigger on partition is not created' => {
 		regexp => qr/CREATE TRIGGER test_trigger.*ON dump_test_second_schema/,
 		like => {},
-		unlike => { %full_runs, %dump_test_schema_runs },
 	},
 
 	# Triggers on partitions should not be dropped individually
@@ -3834,35 +3829,12 @@
 		\QCREATE INDEX measurement_city_id_logdate_idx ON ONLY dump_test.measurement USING\E
 		/xm,
 		like => {
-			binary_upgrade => 1,
-			clean => 1,
-			clean_if_exists => 1,
-			compression => 1,
-			createdb => 1,
-			defaults => 1,
-			exclude_test_table => 1,
-			exclude_test_table_data => 1,
-			no_toast_compression => 1,
-			no_large_objects => 1,
-			no_privs => 1,
-			no_owner => 1,
-			no_table_access_method => 1,
-			only_dump_test_schema => 1,
-			pg_dumpall_dbprivs => 1,
-			pg_dumpall_exclude => 1,
-			schema_only => 1,
+			%full_runs,
+			%dump_test_schema_runs,
 			section_post_data => 1,
-			test_schema_plus_large_objects => 1,
-			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
 			exclude_dump_test_schema => 1,
-			only_dump_test_table => 1,
-			pg_dumpall_globals => 1,
-			pg_dumpall_globals_clean => 1,
-			role => 1,
-			section_pre_data => 1,
 			exclude_measurement => 1,
 		},
 	},
@@ -3913,7 +3885,6 @@
 			role => 1,
 			section_post_data => 1,
 			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
 			exclude_measurement => 1,
@@ -3927,35 +3898,12 @@
 		\QALTER INDEX dump_test.measurement_pkey ATTACH PARTITION dump_test_second_schema.measurement_y2006m2_pkey\E
 		/xm,
 		like => {
-			binary_upgrade => 1,
-			clean => 1,
-			clean_if_exists => 1,
-			compression => 1,
-			createdb => 1,
-			defaults => 1,
-			exclude_dump_test_schema => 1,
-			exclude_test_table => 1,
-			exclude_test_table_data => 1,
-			no_toast_compression => 1,
-			no_large_objects => 1,
-			no_privs => 1,
-			no_owner => 1,
-			no_table_access_method => 1,
-			pg_dumpall_dbprivs => 1,
-			pg_dumpall_exclude => 1,
+			%full_runs,
 			role => 1,
-			schema_only => 1,
 			section_post_data => 1,
 			only_dump_measurement => 1,
-			exclude_measurement_data => 1,
 		},
 		unlike => {
-			only_dump_test_schema => 1,
-			only_dump_test_table => 1,
-			pg_dumpall_globals => 1,
-			pg_dumpall_globals_clean => 1,
-			section_pre_data => 1,
-			test_schema_plus_large_objects => 1,
 			exclude_measurement => 1,
 		},
 	},
@@ -4929,6 +4877,22 @@
 			$test_db = $tests{$test}->{database};
 		}
 
+		# Check for proper test definitions
+		#
+		# There should be a "like" list, even if it is empty.  (This
+		# makes the test more self-documenting.)
+		if (!defined($tests{$test}->{like}))
+		{
+			die "missing \"like\" in test \"$test\"";
+		}
+		# Check for useless entries in "unlike" list.  Runs that are
+		# not listed in "like" don't need to be excluded in "unlike".
+		if ($tests{$test}->{unlike}->{$test_key} &&
+			!defined($tests{$test}->{like}->{$test_key}))
+		{
+			die "useless \"unlike\" entry \"$test_key\" in test \"$test\"";
+		}
+
 		# Skip any collation-related commands if there is no collation support
 		if (!$collation_support && defined($tests{$test}->{collation}))
 		{
-- 
2.42.0

#4Peter Eisentraut
peter@eisentraut.org
In reply to: Peter Eisentraut (#3)
Re: Clean up some pg_dump tests

On 10.10.23 10:03, Peter Eisentraut wrote:

On 09.10.23 11:20, Alvaro Herrera wrote:

I tried this out.  I agree it's a good change.  BTW, this made me
realize that "unlike" is not a good name: maybe it should be called
"except".

right

I would add quotes to the words "like" and "unlike" there.  Otherwise,
these sentences are hard to parse.  Also, some commentary on what this
is about seems warranted: maybe "Check that this test properly defines
which dumps the output should match on." or similar.

Done.

I also moved the code a bit earlier, before the checks for supported
compression libraries etc., so it runs even if those cause a skip.

I didn't like using diag(), because automated runs will not alert to any
problems.  Now maybe that's not critical, but I fear that people would
not notice problems if they are just noise in the output.  Let's make
them test errors.  fail() seems good enough: with the lines I quote
above and omitting the test corrections, I get this, which seems good
enough:

After researching this a bit more, I think "die" is the convention for
problems in the test definitions themselves.  (Otherwise, you're writing
a test about the tests, which would be a bit weird.)  The result is
approximately the same.

committed

#5Peter Eisentraut
peter@eisentraut.org
In reply to: Peter Eisentraut (#1)
1 attachment(s)
Re: Clean up some pg_dump tests

On 02.10.23 09:12, Peter Eisentraut wrote:

1) Remove useless entries from "unlike" lists.  Runs that are not
   listed in "like" don't need to be excluded in "unlike".

2) Ensure there is always a "like" list, even if it is empty.  This
   makes the test more self-documenting.

I also added code that checks 1 and 2 automatically and issues a message
for violations.

I have recently discovered that the same code also exists separately in
the test_pg_dump module test. This should probably be kept consistent.
So here is a patch that adds the same checks there. In this case, we
didn't need to fix any of the existing subtests.

I plan to commit this soon if there are no concerns.

Attachments:

0001-Apply-pg_dump-test-cleanups-to-test_pg_dump-as-well.patchtext/plain; charset=UTF-8; name=0001-Apply-pg_dump-test-cleanups-to-test_pg_dump-as-well.patchDownload
From 0d564bc2ff626b28ef450605d789a0ea42b25992 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 5 Feb 2024 11:22:01 +0100
Subject: [PATCH] Apply pg_dump test cleanups to test_pg_dump as well

Apply the changes from 41a284411e0 to the test_pg_dump module as well.
Here, we just apply the new test consistency checks, but we don't need
to fix any existing tests.
---
 src/test/modules/test_pg_dump/t/001_base.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index 339e7459ca7..b8c30c23872 100644
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -857,6 +857,22 @@
 
 	foreach my $test (sort keys %tests)
 	{
+		# Check for proper test definitions
+		#
+		# There should be a "like" list, even if it is empty.  (This
+		# makes the test more self-documenting.)
+		if (!defined($tests{$test}->{like}))
+		{
+			die "missing \"like\" in test \"$test\"";
+		}
+		# Check for useless entries in "unlike" list.  Runs that are
+		# not listed in "like" don't need to be excluded in "unlike".
+		if ($tests{$test}->{unlike}->{$test_key}
+			&& !defined($tests{$test}->{like}->{$test_key}))
+		{
+			die "useless \"unlike\" entry \"$test_key\" in test \"$test\"";
+		}
+
 		# Run the test listed as a like, unless it is specifically noted
 		# as an unlike (generally due to an explicit exclusion or similar).
 		if ($tests{$test}->{like}->{$test_key}
-- 
2.43.0