-
Notifications
You must be signed in to change notification settings - Fork 219
Container PyTest suite for Postgresql-container #640
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Petr "Stone" Hracek <[email protected]>
The migration matrix is following: run_container_creation_tests -> test_container_configuration.py run_general_tests -> test_container_general.py run_change_password_test -> test_container_password.py run_replication_test -> test_container_replication.py run_s2i_test -> test_container_basics.py run_test_cfg_hook -> test_container_configuration.py run_s2i_bake_data_test -> test_container_ssl.py run_s2i_enable_ssl_test -> test_container_ssl.py run_pgaudit_test -> test_container_extensions.py run_pgvector_test -> test_container_extensions.py run_env_extension_load_test -> test_container_extensions.py run_logging_test -> test_container_extensions.py The following tests are NOT migrated yet. run_migration_test and run_upgrade_tests. Signed-off-by: Petr "Stone" Hracek <[email protected]>
Pull Request validationFailed🔴 Review - Missing review from a member (1 required) Success🟢 CI - All checks have passed |
|
Let's try first round |
Testing Farm results
|
Don not call the function twice Signed-off-by: Petr "Stone" Hracek <[email protected]>
d7126c2 to
a7a86fd
Compare
|
Let's try next round |
| def test_correct_configuration_tests( | ||
| self, | ||
| psql_user, | ||
| psql_password, | ||
| psql_database, | ||
| psql_admin_password, | ||
| ): | ||
| """ | ||
| Test correct configuration combinations for PostgreSQL container. | ||
| """ | ||
| assert self.db.assert_container_creation_succeeds( | ||
| container_args=[ | ||
| psql_user, | ||
| psql_password, | ||
| psql_database, | ||
| psql_admin_password, | ||
| ], | ||
| command="", | ||
| ) |
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.
Unlike in the bash tests, this doesn't try to actually connect to the container after creation as test_connection_func and connection_params are missing. This causes a case that fails in the bash tests to pass here.
Reproducer:
psql_user="the@user"
psql_password="pass"
psql_database="db"Output from pytests:
PASSED test_container_configuration.py::TestPostgreSQLConfigurationContainer::test_correct_configuration_tests[-e POSTGRESQL_USER="the@user"--e POSTGRESQL_PASSWORD="the pass"--e POSTGRESQL_DATABASE="the db"--e POSTGRESQL_ADMIN_PASSWORD=]
Output from bash tests:
Created container 99890eb43cc9bc0a105aa15ef49954f88516d4bd16da3e1058786519646207e9
Testing PostgreSQL connection to 10.88.8.202...
Trying to connect...
psql: error: could not translate host name "[email protected]" to address: Name or service not known
Obviously the bash tests don't handle it exactly correctly here, because input into the connection URI should be URI-encoded. But I would suggest adding a connection attempt here, too.
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'd also recommend adding a test case for this, as the only reason I noticed this error was that pg18 complains about spaces in usernames and db names inside the connection URI, but other special character such as the @ were problematic before, too)
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 introduce a way to fix this behaviour for the bash tests in #641. Also I added a new test there for special characters directly.
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.
Makes sense to me. I will do it.
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.
Fixed by 8f8b048
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 case now fails because the container is created correctly with environment variables, but the connection done with postgres://the@user@ip:5432/the/db obviously fails.
[
'"the@user"',
'"the pass"',
'"the/db"',
"",
],
I'd say this is a discrepancy and the test should account for this by URI encoding the username and db name for consistency.
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.
But that code is in container_lib
function Signed-off-by: Petr "Stone" Hracek <[email protected]>
This pull request adds PyTest suite migrated from
run_pytestscripts.All tests are migrated except
run_migration_testandrun_upgrade_test.