Adding helper functions that reduce code duplications in TAP tests - #263
Adding helper functions that reduce code duplications in TAP tests#263shahidullah79 wants to merge 3 commits into
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project status has failed because the head coverage (78.30%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## TDE_REL_17_STABLE #263 +/- ##
=====================================================
+ Coverage 78.13% 78.30% +0.17%
=====================================================
Files 22 22
Lines 2488 2485 -3
Branches 393 392 -1
=====================================================
+ Hits 1944 1946 +2
+ Misses 466 462 -4
+ Partials 78 77 -1
🚀 New features to boost your workflow:
|
|
I am generally opposed to these types of helpers in tests. For test code, it's very important that it's easy to read and understand, and less important that it's easy to write. DRY principles doesn't really apply to tests. I don't think this is a direction that would make our TAP tests better. That said, adding helpers for setting up common scaffolding could possibly be useful. But executing the test subject and asserting on it shouldn't be hidden behind indirection imho. |
| } | ||
|
|
||
| # Set up pg_tde extension and add a database key provider and set the database key | ||
| sub setup_pg_tde_db_environment |
There was a problem hiding this comment.
Here and the previous function: maybe _key or _keyring instead of _environment? E.g. setup_pg_tde_global_key and setup_pg_tde_db_key
| # Set up pg_tde extension and add a database key provider and set the database key | ||
| sub setup_pg_tde_db_environment | ||
| { | ||
| my ($node, $key_name, $provider_name, $provider_path) = @_; |
There was a problem hiding this comment.
In most cases in tests nobody cares about the key and provider name, and path. So maybe have defaults like:
my $testname = basename($0);
$testname =~ s/\.[^.]+$//;
my $key_name = $key_name . '_db_key';
my $provider_name = $testname . '_provider';
my $provider_path = '/tmp/' . $testname . '.per';
And then in most cases you call much more convenient setup_pg_tde_db_key($node)
| sub enable_pg_tde_in_conf | ||
| { | ||
| my ($node) = @_; | ||
| $node->append_conf('postgresql.conf', "shared_preload_libraries = 'pg_tde'"); | ||
| } | ||
|
|
||
| # Set default table access method to tde_heap | ||
| sub set_default_table_am_tde_heap | ||
| { | ||
| my ($node) = @_; | ||
| $node->append_conf('postgresql.conf', "default_table_access_method = 'tde_heap'"); | ||
| } |
There was a problem hiding this comment.
I'm not sure about those two functions. I don't think anyone would memorize this func names, hence they would be copied from the existing tests anyway. But then it's not a bit difference which line is to copy (I mean $node->append_conf('postgresql.conf', "default_table_access_method = 'tde_heap'"); or set_default_table_am_tde_heap($node)). But append_conf( is more clear of what it does. However, I don't have super strong opinion about that.
| my ($node, $table_name, $expected) = @_; | ||
| my $result = safe_psql('postgres', "SELECT pg_tde_is_encrypted('$table_name')"); | ||
| append_to_result_file($node->name . ": encryption check result for $table_name = $result"); | ||
| is($result, $expected, "Check encryption status for '$table_name' on " . $node->name); |
There was a problem hiding this comment.
I suggest not having such a test assertion (anything we use from Test::More) as part of a helper function; this should be handled in the main test file. That will make it easier to understand and follow the test case.
| append_to_result_file("-- server restart with wal encryption = $value"); | ||
| $node->restart; | ||
| } | ||
|
|
There was a problem hiding this comment.
Having such helper functions for scaffolding purposes is a good idea, will reduce the clutter in the test files and look better.
5 participants
I have added few helper functions that will reduce code duplication in TAP tests. like
Go through these functions if you think these will be useful then will merge it.