Skip to content

Proof of concept: Ideas for cleaner TAP tests #249

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Proof of concept: Ideas for cleaner TAP tests #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfd3b4e
Remove tests that can easily be done by pg_regrss
AndersAstrand Apr 23, 2025
21dc7e2
Do not assert that test scaffolding works
AndersAstrand Apr 23, 2025
b94a41f
Use Test::More assertions
AndersAstrand Apr 23, 2025
856e6e8
Reformat SQL to make it easier to read
AndersAstrand Apr 23, 2025
7f475e9
Add wrapper to make Andreas happy
AndersAstrand Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 48 additions & 55 deletions contrib/pg_tde/t/001_basic.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,79 +1,72 @@
#!/usr/bin/perl

use strict;
use warnings;
use File::Basename;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
use lib 't';
use pgtde;

PGTDE::setup_files_dir(basename($0));
use FindBin;
use lib $FindBin::RealBin;

use pgtde;

my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
$node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'");
$node->start;

PGTDE::psql($node, 'postgres', 'CREATE EXTENSION IF NOT EXISTS pg_tde;');

PGTDE::psql($node, 'postgres',
'SELECT extname, extversion FROM pg_extension WHERE extname = \'pg_tde\';'
PGTDE::do_psql($node, 'postgres', q{CREATE EXTENSION IF NOT EXISTS pg_tde});
PGTDE::do_psql(
$node, 'postgres', q{
SELECT pg_tde_add_database_key_provider_file(
provider_name => 'file-vault',
file_path => '/tmp/pg_tde_test_keyring.per'
)
}
);

PGTDE::psql($node, 'postgres',
'CREATE TABLE test_enc(id SERIAL,k INTEGER,PRIMARY KEY (id)) USING tde_heap;'
PGTDE::do_psql(
$node, 'postgres', q{
SELECT pg_tde_set_key_using_database_key_provider(
key_name => 'test-db-key',
provider_name => 'file-vault'
)
}
);

PGTDE::append_to_result_file("-- server restart");
$node->restart;

PGTDE::psql($node, 'postgres',
"SELECT pg_tde_add_database_key_provider_file('file-vault','/tmp/pg_tde_test_keyring.per');"
PGTDE::do_psql(
$node, 'postgres', q{
CREATE TABLE test_enc(
id SERIAL,
k VARCHAR(32),
PRIMARY KEY (id)
) USING tde_heap
}
);

PGTDE::psql($node, 'postgres',
"SELECT pg_tde_set_key_using_database_key_provider('test-db-key','file-vault');"
PGTDE::do_psql(
$node, 'postgres', q{
INSERT INTO test_enc (k) VALUES ('foobar'), ('barfoo')
}
);

PGTDE::psql($node, 'postgres',
'CREATE TABLE test_enc(id SERIAL,k VARCHAR(32),PRIMARY KEY (id)) USING tde_heap;'
);

PGTDE::psql($node, 'postgres',
'INSERT INTO test_enc (k) VALUES (\'foobar\'),(\'barfoo\');');

PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id ASC;');

PGTDE::append_to_result_file("-- server restart");
$node->restart;

PGTDE::psql($node, 'postgres', 'SELECT * FROM test_enc ORDER BY id ASC;');

# Verify that we can't see the data in the file
my $tablefile = $node->safe_psql('postgres', 'SHOW data_directory;');
$tablefile .= '/';
$tablefile .=
$node->safe_psql('postgres', 'SELECT pg_relation_filepath(\'test_enc\');');

my $strings = 'TABLEFILE FOUND: ';
$strings .= `(ls $tablefile >/dev/null && echo yes) || echo no`;
PGTDE::append_to_result_file($strings);
is( PGTDE::do_psql(
$node, 'postgres', q{
SELECT * FROM test_enc ORDER BY id ASC
}
),
"1|foobar\n2|barfoo",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I wrote directly to Anders I worry that this kind of comparison will result in a pretty hard to read output and that we need to split the output but in general I think this is moving us in the right direction. Need to test this locally a bit tomorrow and see if we are fine with it as-is or if we need to write the better helper already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the "output comparison" style tests, but maybe that's because I'm used to how mysql works - there the test framework is a combination of the SQL tests and the tap tests, basically the SQL tests have way more features.

Here the only reason why we have to use tap at all is because the sql tests are missing some features, mainly proper server restarts.

Copy link
Collaborator Author

@AndersAstrand AndersAstrand Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that style of test just leads to higher maintenance cost without giving any value. I would probably prefer if we mainly did TAP tests, so I understand I'm the odd man out.

I just don't see the appeal of having to update the expectations for every single test if you change the output of a function, even if that function is only run as part of setting up scaffolding for every test except the one that actually tests that function.

I think there only need to be one test that asserts that CREATE EXSTENSION pg_tde works for example, and the rest can just assume that it works and focus on asserting whatever their test subject is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are two examples of the high maintenance cost of the regression tests I've fixed recently:

Test didn't actually test anything, it only setup scaffolding: ce63840
Test asserted the complete opposite of what it claimed to test: 1390dd0

These issues are more or less unavoidable in regression tests, but with proper TAP tests we can at least avoid them there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is annoying, mysql mtr has special commands for that, disable_query_log, enable_query_log and disable_result_log, enable_result_log - statements written between those, or their results, won't get written to the output file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach that was suggested. This is the proper way of writing the TAP test cases, and we should incline towards moving forward. The individual TAP test should only focus on asserting that the test subject is, and use Test::More. @AndersAstrand, you are not the only odd man out. :)

Maintaining expectations is way too much for extra unnecessary work that we can avoid. Using comparison-based TAP in PGSM had its own merits, but it also brought the overhead of additional maintenance work. Using the file comparison approach, we might also have to maintain PG Server Version (in case) or OS-specific output files, just like PGSM.

We can tweak the wrapper function to generate the debug_out files in the results directory for each step or test case for debugging purposes or checking the outputs. These files could be helpful if something is broken or on a need-to-know basis, but not for comparing output files. I am referring to something like this.
`
sub do_psql
{
my ($node, $dbname, $sql, %params) = @_;

local %ENV = $node->_get_env();

my ($stdout, $stderr);
my $ret = $node->psql(
	$dbname, $sql,
	%params,
	stdout => \$stdout,
	stderr => \$stderr,
	on_error_die => 0,
	on_error_stop => 1);

if ($stdout ne '')
{
	append_to_debug_file($stdout);
}

if ($stderr ne '')
{
	append_to_debug_file($stderr);
}

# psql can emit stderr from NOTICEs etc
if ($stderr ne "")
{
	diag("#### Begin standard error\n");
	diag($stderr);
	diag("\n#### End standard error\n");
}

die() if $ret;
return $stdout;

}
`

'tde_heap table can be read after server restart');

$strings = 'CONTAINS FOO (should be empty): ';
$strings .= `strings $tablefile | grep foo`;
PGTDE::append_to_result_file($strings);
my $tablefile =
$node->data_dir . '/'
. PGTDE::do_psql($node, 'postgres',
q{SELECT pg_relation_filepath('test_enc')});
my $tablefilecontents = slurp_file($tablefile);

PGTDE::psql($node, 'postgres', 'DROP TABLE test_enc;');

PGTDE::psql($node, 'postgres', 'DROP EXTENSION pg_tde;');
unlike($tablefilecontents, qr/foo/,
'table file does not contain plaintext data');

$node->stop;

# Compare the expected and out file
my $compare = PGTDE->compare_results();

is($compare, 0,
"Compare Files: $PGTDE::expected_filename_with_path and $PGTDE::out_filename_with_path files."
);

done_testing();
45 changes: 0 additions & 45 deletions contrib/pg_tde/t/expected/001_basic.out
View file
Open in desktop

This file was deleted.

27 changes: 27 additions & 0 deletions contrib/pg_tde/t/pgtde.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,31 @@ sub compare_results
return compare($expected_filename_with_path, $out_filename_with_path);
}

sub do_psql
{
my ($node, $dbname, $sql, %params) = @_;

local %ENV = $node->_get_env();

my ($stdout, $stderr);
my $ret = $node->psql(
$dbname, $sql,
%params,
stdout => \$stdout,
stderr => \$stderr,
on_error_die => 0,
on_error_stop => 1);

# psql can emit stderr from NOTICEs etc
if ($stderr ne "")
{
diag("#### Begin standard error\n");
diag($stderr);
diag("\n#### End standard error\n");
}

die() if $ret;

return $stdout;
}
1;