-
Notifications
You must be signed in to change notification settings - Fork 26
[DPE-4068] Convert all tests from unittest to pytest + enable/disable secrets in every test #452
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
Conversation
Typically: to save CI time, when the same check were executed in a Juju 3-specific way already | ||
""" | ||
if juju_has_secrets: | ||
pytest.skip("Skipping legacy secrets tests") |
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.
We had already the whole test suite running with and without secrets (unit-juju2
and unit-juju3
respectively), the only change needed was to adapt the few tests that only ran in one or the other, and remove this fixtures.
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.
merging those 2 suites into one will be addressed in a follow-up PR!
@patch_network_get(private_address="1.1.1.1") | ||
@patch("charm.PostgresqlOperatorCharm._on_leader_elected") | ||
@pytest.mark.usefixtures("use_caplog") | ||
def test_delete_existing_password_secrets(self, _, __): |
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.
similar to what was done in canonical/postgresql-operator#425, this test got removed and test_delete_password
runs with and without secrets
): | ||
# as this test checks for a migration from databag to secrets, | ||
# there's no need for this test when secrets are not enabled. | ||
if not juju_has_secrets: |
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.
here and on the test below, same idea from canonical/postgresql-operator#425
tc.assertEqual(len(first_password), 16) | ||
tc.assertIsNotNone(re.fullmatch("[a-zA-Z0-9\b]{16}$", first_password)) |
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.
We can just use regular assert
s in such cases. No need to keep the test case just for that.
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.
agreed, will refactor this together with merging the 2 suites in the follow-up PR.
This reverts commit 9137e67.
… secrets in every test (canonical#452) * convert all tests to pytest * remove secret remove hook * Revert "remove secret remove hook"
Follow-up to DPE-2674 in canonical/postgresql-operator#425