Add PyTest suite to mariadb-container #311
Conversation
Migration matrix is: - run_container_creation_tests -> test_container_configuration.py(test_try_image_invalid_combinations, test_container_creation_fails, test_invalid_configuration_tests.py - run_configuration_tests -> test_container_configuration.py(TestMariaDBConfigurationTests and TestsMariaDBConfiguratioTest) TestMariaDB - run_general_tests -> test_container_general.py(test_run and test_datadir) - run_change_password_test -> test_container_password.py - run_change_password_new_user_test -> test_container_password.py - run_replication_test -> test_container_replication.py - run_s2i_test -> test_container_basics.py - run_ssl_test -> test_container_basics.py - run_upgrade_test -> test_container_upgrade.py - run_plugin_test -> test_container_plugin.py Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
[test-pytest] |
Pull Request validationFailed🔴 Review - Missing review from a member (1 required) Success🟢 CI - All checks have passed |
Testing Farm results
|
Lets' connect to proper DB including --disable certificates Let's check if Slave is really runnning before SQL command is called. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
[test-pytest] |
Call sql commands at once. Change initial names to understandable. Use DatabaseWrapper directly to call sql commands. Check mariadb configuration values together. Do not duplicate code. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
re-trigger tests after a hude refactoring. [test][test-pytest] |
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
phracek
force-pushed
the
container_pytests
branch
from
6bd423f to
f990c37
Compare
|
re-trigger tests after fixing issue on Fedora [test][test-pytest] |
and parametrized it. It is easier to review code. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
[test][test-pytest] |
|
@frenzymadness PTAL in case you have free time. |
frenzymadness
left a comment
frenzymadness
left a comment
There was a problem hiding this comment.
This PR adds 1355 lines of code so it will take a while to review it. I'm submitting what I have so far so we can ping-pong small portions of it.
is defined in VARS variable directly Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Update also description for better reading what each test does in case it is huge Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
if mariadb contains proper settings Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
The function is already part of container-ci-suite. Add several documentation part to the tests Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
phracek
force-pushed
the
container_pytests
branch
from
d218b68 to
2d65c49
Compare
|
@frenzymadness I have tryied to address all issues. As soon as you will have a spare time. Please look at this. Next time, we can add each test by simple PR for better review. It makes sense for me to PostgreSQL container. |
|
[test-pytest][test] |
frenzymadness
left a comment
frenzymadness
left a comment
There was a problem hiding this comment.
I've found some possible improvements, but nothing really serious, so do whatever you prefer with them and merge this PR then. Sorry it took me so long to do the review.
| @pytest.mark.parametrize( | ||
| "action", | ||
| [ | ||
| "", | ||
| "analyze", | ||
| "optimize", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
I don't understand how the different values here affect the containers. I see the test uses a different name for cid file but what is the real impact?
| def test_password_change(self, username, password, pwd_change): | ||
| """ | ||
| Test password change. | ||
| """ | ||
|
|
||
| self.password_change_test( | ||
| username=username, | ||
| password=password, | ||
| pwd_dir=pwd_dir_change, | ||
| pwd_change=pwd_change, | ||
| ) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "username, password, user_change", | ||
| [ | ||
| ("user", "foo", False), | ||
| ("user2", "bar", True), | ||
| ], | ||
| ) | ||
| def test_password_change_new_user_test(self, username, password, user_change): | ||
| """ | ||
| Test user change. | ||
| """ | ||
| self.password_change_test( | ||
| username=username, | ||
| password=password, | ||
| pwd_dir=user_dir_change, | ||
| user_change=user_change, | ||
| ) | ||
|
|
There was a problem hiding this comment.
What is the benefit of having two tests with different parameters calling the same function with the same logic? Cannot we have one test with all parameters combined and full logic inside?
| 1. Create a container with the given arguments | ||
| 2. Check if the container is created successfully | ||
| 3. Check if the database connection works | ||
| 4. Check if the userchange, then user2 does exist in the database | ||
| 5. Check if the password works | ||
| 6. Check if the password does not work |
There was a problem hiding this comment.
Sorry, but are those comments created by AI? I find very little value in them. The test is kinda complex and a comment like this does not help at all.
|
|
||
| def test_plugin_installation(self): | ||
| """ | ||
| Test plugin installation. |
There was a problem hiding this comment.
Just an example of a useful comment for this test: "Test installs a plugin that stores error messages in a log file and then checks that the plugin works correctly".
| slave_found = False | ||
| for _ in range(3): | ||
| result = self.db_wrapper_api.run_sql_command( | ||
| container_ip=master_cip, | ||
| username="root", | ||
| password="root", | ||
| container_id=master_cid, | ||
| database=f"db {VARS.SSL_OPTION}", | ||
| sql_cmd="SHOW SLAVE HOSTS;", | ||
| podman_run_command="exec", | ||
| ) | ||
| if not result: | ||
| sleep(3) | ||
| continue | ||
| if slave_cip in result: | ||
| slave_found = True | ||
| break | ||
| sleep(3) | ||
| assert slave_found |
There was a problem hiding this comment.
This can be simplified to something like:
for _ in range(3):
result = …
if slave_cip in result:
break
sleep(3)
else:
assert False, "Slave IP not found!"If you break from for loop, the else block is ignored.
| "Slave_IO_Running:\\s*Yes", | ||
| "Slave_SQL_Running:\\s*Yes", |
There was a problem hiding this comment.
| "Slave_IO_Running:\\s*Yes", | |
| "Slave_SQL_Running:\\s*Yes", | |
| r"Slave_IO_Running:\s*Yes", | |
| r"Slave_SQL_Running:\s*Yes", |
| # sql_cmd = "select * from t1;" | ||
| # mysql_cmd = f'mysql -uroot <<< "{sql_cmd}"' | ||
| # table_output = PodmanCLIWrapper.call_podman_command( | ||
| # cmd=f"exec {slave_cid} bash -c '{mysql_cmd}'", | ||
| # ) |
| """ | ||
| Test SSL. | ||
| """ |
There was a problem hiding this comment.
Another example - the comment is not very useful if it's a 1:1 copy of the name of the test function.
| assert not re.search("Running mysql_upgrade", output), ( | ||
| "mysql_upgrade did not run" |
There was a problem hiding this comment.
Reversed logic: we expect NOT to find "Running mysql_upgrade", and if we find it, the error message should say something like: "Unexpected mysql_upgrade found".
2 participants
Migration matrix is: