-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
PG-1289: Add testcase to cover PGMajor.PGMinor.PerconaVersion #150
PG-1289: Add testcase to cover PGMajor.PGMinor.PerconaVersion #150
Conversation
Added a file ci_scripts/make-test-tde-only.sh to test pg_tde test suite only. Added lcov to apt install list for coverage and enable-coverage flag in configure.
@@ -0,0 +1,19 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this script is misleading, it's not tde-only
but "only main regression suite with tde, without contrib".
Also it's a duplication of the other script, can't we make this an option to the existing make-test-tde
script?
fi | ||
|
||
cd $SCRIPT_DIR/../contrib/pg_tde | ||
pwd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug leftover?
ok($rt_value == 1, "Start Server"); | ||
|
||
# CREATE EXTENSION and change out file permissions | ||
my ($cmdret, $stdout, $stderr) = $node->psql('postgres', 'CREATE EXTENSION pg_tde;', extra_params => ['-a']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the extension for this test? Why is this test in the extension folder?
set -e | ||
|
||
# This script is used to configure a TDE server for testing purposes. | ||
export TDE_MODE=1 | ||
export PERCONA_SERVER_VERSION=17.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the variable for the configure command? It is only used by test commands
@@ -0,0 +1,85 @@ | |||
#!/usr/bin/perl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file isn't executed at all, no related makefile or meson.build changes.
@@ -1,6 +1,7 @@ | |||
#!/bin/bash | |||
|
|||
export TDE_MODE=1 | |||
export PERCONA_SERVER_VERSION=17.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the variable for the build command?
@@ -3,6 +3,7 @@ | |||
set -e | |||
|
|||
export TDE_MODE=1 | |||
export PERCONA_SERVER_VERSION=17.4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this repeating.. With this, every time we update the version, we have to replace it at many places.
Can't we add it as a file called PERCONA_SERVER_VERSION
, if we really want to do this?
Also, only the make scripts are updated, the meson scripts are left as-is.
Added a file ci_scripts/make-test-tde-only.sh to test pg_tde test suite only.
Added lcov to apt install list for coverage and enable-coverage flag in configure.