ipatests: test_installation: add install test scenarios #5610

miskopo · 2021-03-05T10:41:10Z

test_hostname_parameter: Test for issue 2692 ipa-server-install ignores --hostname:
check whether hostname provided in --hostname parameter is being taken into account and set as new hostname without prompting for it again

test_ad_subpackage_dependency: Test for issue 4011 ipa-server-install crashes when AD subpackage is not installed:
test if ipa-server installation succeeds without freeipa-ipa-server-trust-ad installed

test_backup_of_cs_cfg_should_be_created: Test for issue 4166 Backup CS.cfg before modifying it:
test if ipa-server installer backs up CS.cfg before modifying it

test_installer_wizard_should_prompt_for_DNS: Test for issue 2575 [RFE] Installer wizard should prompt for DNS:
test if installer is asking for DNS setup details if not provided as parameter

Related: https://pagure.io/freeipa/issue/2692
Related: https://pagure.io/freeipa/issue/4011
Related: https://pagure.io/freeipa/issue/4166
Related: https://pagure.io/freeipa/issue/2575

Signed-off-by: Michal Polovka mpolovka@redhat.com

ipatests/test_integration/test_installation.py

miskopo · 2021-03-08T11:12:07Z

I've noticed an error in test_installer_wizard_should_prompt_for_DNS, I'll resolve with newest update

ipatests/test_integration/test_installation.py

wladich

Two new test cases do not deal with the installation process but rather check the rusult: test_update_krb5_conf_template_with_keyring_default_ccache_name and
test_backup_of_cs_cfg_should_be_created. Do you think those could be integrated to some other tests to save two ipa-server installations? This could save about 20 for of test execution.

ipatests/test_integration/test_installation.py

miskopo · 2021-03-30T11:42:34Z

Two new test cases do not deal with the installation process but rather check the rusult: test_update_krb5_conf_template_with_keyring_default_ccache_name and
test_backup_of_cs_cfg_should_be_created. Do you think those could be integrated to some other tests to save two ipa-server installations? This could save about 20 for of test execution.

That's true, however, each test should test one thing and not be dependent on other test - if I merged these two test case into, e.g. test_install_master, I could obfuscate failures in one of the test cases, depending on which is going to be first.

wladich

In added code you are using both types of quotes - single and double. According to PEP 8 it is advisable to stick to one type of quotes. This does not apply do docstrings, they should always use double quotes.

ipatests/test_integration/test_installation.py

wladich · 2021-04-23T13:55:06Z

ipatests/test_integration/test_installation.py

+        tasks.install_master(self.master)
+        tasks.kinit_admin(self.master)
+        klist = self.master.run_command(['klist']).stdout_text
+        assert '/tmp/' not in klist


You are checking two conditions:

template is updated to use cache

cache is used poperly

Second condition can be checked with test cases:

Proper cache is used

Improper cache is not used.

You test only TC2. I suggest that TC1 should be checked as well,

miskopo · 2021-04-28T10:48:51Z

@wladich I am really sorry, but I am once again asking you for a review. I am, however, eternally grateful for what you do.

miskopo · 2021-04-29T11:37:04Z

as seen in failed temp commit, klist doesn't use KEYRING, but KCM in this particular situation.

wladich · 2021-05-06T08:08:09Z

@miskopo Please do check if timeouts in prci definitions need to be adjusted.

miskopo · 2021-05-06T08:11:50Z

@miskopo Please do check if timeouts in prci definitions need to be adjusted.

Both TestInstallMaster and TestInstallMasterDNS have plenty of extra time, more than 16 minutes in gating and more than 2 hours in nightlies.

wladich

Please rewrite commit message to avoid GitHub from interpretting Pagure issues as GH PRs
Please see few minor issues in inline comments. Otherwise LGTM

wladich · 2021-05-06T08:16:15Z

ipatests/test_integration/test_installation.py

+            ['ipa', 'dnszone-show', self.master.domain.name]
+        ).stdout_text
+
+        assert "Active zone: TRUE" in result


Inconsistent usage of double quotes

wladich · 2021-05-06T08:17:45Z

ipatests/test_integration/test_installation.py

+                'hostname', '-f']).stdout_text.strip()
+            assert hostname == new_hostname
+        finally:
+            # no need to restore the hostane as the installer


Typo: "hostane" -> "hostname"

wladich · 2021-05-06T08:48:23Z

ipatests/test_integration/test_installation.py

+            if reinstall:
+                tasks.install_packages(self.master, [package_name])
+
+    def test_backup_of_cs_cfg_should_be_created(self, server_cleanup):


Same as with test_installer_wizard_prompts_for_DNS: proper form of the phrase is "test backup is created" or "test creates backup"

wladich

Removing "LGTM". Looking through the logs I found the incorrect invocation of rpm

wladich · 2021-05-06T11:08:31Z

ipatests/test_integration/test_installation.py

+        """
+        package_name = '*ipa-server-trust-ad'
+        reinstall = False
+        if tasks.is_package_installed(self.master, package_name):


This internaly calls 'rpm' '-q' '*ipa-server-trust-ad'. Unlike dnf the rpm does not expand wildcards and this check will allways return False

ipatests/test_integration/test_installation.py

test_hostname_parameter: Test for issue 2692 ipa-server-install ignores --hostname: check whether hostname provided in `--hostname` parameter is being taken into account and set as new hostname without prompting for it again test_ad_subpackage_dependency: Test for issue 4011 ipa-server-install crashes when AD subpackage is not installed: test if ipa-server installation succeeds without `freeipa-ipa-server-trust-ad` installed test_backup_of_cs_cfg_should_be_created: Test for issue 4166 Backup CS.cfg before modifying it: test if ipa-server installer backs up CS.cfg before modifying it test_installer_wizard_should_prompt_for_DNS: Test for issue 2575 [RFE] Installer wizard should prompt for DNS: test if installer is asking for DNS setup details if not provided as parameter Related: https://pagure.io/freeipa/issue/2692 Related: https://pagure.io/freeipa/issue/4011 Related: https://pagure.io/freeipa/issue/4166 Related: https://pagure.io/freeipa/issue/2575 Signed-off-by: Michal Polovka <mpolovka@redhat.com>

miskopo · 2021-05-06T13:28:32Z

wladich · 2021-05-06T13:29:26Z

LGTM #2
Please remove the temp commit and I will ACk

miskopo · 2021-05-06T14:52:47Z

master:

b8ebce7 ipatests: test_installation: add install test scenarios

miskopo added the needs review Pull Request is waiting for a review label Mar 5, 2021

miskopo self-assigned this Mar 5, 2021

miskopo requested a review from fcami March 5, 2021 10:41

miskopo force-pushed the install-server-cli branch from eea8176 to 98050df Compare March 5, 2021 12:05

rcritten reviewed Mar 5, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

rcritten reviewed Mar 5, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

rcritten reviewed Mar 5, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

miskopo force-pushed the install-server-cli branch from 98050df to 2e8061a Compare March 6, 2021 16:04

miskopo removed the request for review from fcami March 6, 2021 16:11

miskopo force-pushed the install-server-cli branch 7 times, most recently from c0d4967 to 1d1901c Compare March 7, 2021 16:35

This comment has been minimized.

Sign in to view

miskopo force-pushed the install-server-cli branch 2 times, most recently from b5ad884 to a7d789c Compare March 8, 2021 10:36

wladich reviewed Mar 8, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

miskopo commented Mar 8, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

wladich reviewed Mar 8, 2021

View reviewed changes

ipatests/test_integration/test_installation.py Outdated Show resolved Hide resolved

miskopo force-pushed the install-server-cli branch 5 times, most recently from 79988ab to 8b07b59 Compare March 31, 2021 09:25

wladich reviewed Apr 23, 2021

View reviewed changes

miskopo force-pushed the install-server-cli branch 2 times, most recently from efacd04 to b8ca378 Compare April 28, 2021 09:44

miskopo force-pushed the install-server-cli branch from b8ca378 to 5ddb92d Compare April 29, 2021 09:59

miskopo force-pushed the install-server-cli branch 2 times, most recently from 4ca4871 to 19f21d8 Compare May 3, 2021 13:53

miskopo added the re-run Trigger a new run of PR-CI label May 3, 2021

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label May 3, 2021

miskopo force-pushed the install-server-cli branch from 19f21d8 to 22968d9 Compare May 6, 2021 06:44

wladich reviewed May 6, 2021

View reviewed changes

miskopo force-pushed the install-server-cli branch from 22968d9 to f8b9c1f Compare May 6, 2021 09:55

wladich requested changes May 6, 2021

View reviewed changes

miskopo force-pushed the install-server-cli branch from f8b9c1f to c5b2887 Compare May 6, 2021 11:43

miskopo force-pushed the install-server-cli branch from c5b2887 to f1579a3 Compare May 6, 2021 13:29

wladich added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels May 6, 2021

miskopo added ipa-4-8 Mark for backport to ipa 4.8 ipa-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed labels May 6, 2021

miskopo closed this May 6, 2021

miskopo mentioned this pull request May 6, 2021

[Backport][ipa-4-9] ipatests: test_installation: add install test scenarios #5753

Closed

miskopo removed the ipa-4-8 Mark for backport to ipa 4.8 label May 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipatests: test_installation: add install test scenarios #5610

ipatests: test_installation: add install test scenarios #5610

miskopo commented Mar 5, 2021 •

edited

This comment has been minimized.

miskopo commented Mar 8, 2021

wladich left a comment

miskopo commented Mar 30, 2021

wladich left a comment

wladich Apr 23, 2021

miskopo commented Apr 28, 2021

miskopo commented Apr 29, 2021 •

edited

wladich commented May 6, 2021

miskopo commented May 6, 2021

wladich left a comment

wladich May 6, 2021

wladich May 6, 2021

wladich May 6, 2021

wladich left a comment

wladich May 6, 2021

miskopo commented May 6, 2021

wladich commented May 6, 2021 •

edited

miskopo commented May 6, 2021

ipatests: test_installation: add install test scenarios #5610

ipatests: test_installation: add install test scenarios #5610

Conversation

miskopo commented Mar 5, 2021 • edited

This comment has been minimized.

miskopo commented Mar 8, 2021

wladich left a comment

Choose a reason for hiding this comment

miskopo commented Mar 30, 2021

wladich left a comment

Choose a reason for hiding this comment

wladich Apr 23, 2021

Choose a reason for hiding this comment

miskopo commented Apr 28, 2021

miskopo commented Apr 29, 2021 • edited

wladich commented May 6, 2021

miskopo commented May 6, 2021

wladich left a comment

Choose a reason for hiding this comment

wladich May 6, 2021

Choose a reason for hiding this comment

wladich May 6, 2021

Choose a reason for hiding this comment

wladich May 6, 2021

Choose a reason for hiding this comment

wladich left a comment

Choose a reason for hiding this comment

wladich May 6, 2021

Choose a reason for hiding this comment

miskopo commented May 6, 2021

wladich commented May 6, 2021 • edited

miskopo commented May 6, 2021

miskopo commented Mar 5, 2021 •

edited

miskopo commented Apr 29, 2021 •

edited

wladich commented May 6, 2021 •

edited