Nir Soffer has posted comments on this change. Change subject: infra tests: added functional test for upgrading vdsm ......................................................................
Patch Set 10: (7 comments) https://gerrit.ovirt.org/#/c/61186/10/tests/functional/upgrade_vdsm_test.py File tests/functional/upgrade_vdsm_test.py: Line 24: Line 25: repo_url = 'http://resources.ovirt.org/pub/ovirt-3.6/rpm/fc22/' Line 26: Line 27: Line 28: def run_command(command, out_pipe=subprocess.PIPE, err_pipe=subprocess.PIPE): Using same arguments names as the underlying api (stdout, stderr) is better, make it easier to understand what you are trying to do, but in this case we don't need these, so best would be to remove this arguments and use stdout=subprocess.PIPE... Line 29: p = subprocess.Popen(command, stdout=out_pipe, stderr=err_pipe) Line 30: out, err = p.communicate() Line 31: if p.returncode != 0: Line 32: raise Exception(err) Line 28: def run_command(command, out_pipe=subprocess.PIPE, err_pipe=subprocess.PIPE): Line 29: p = subprocess.Popen(command, stdout=out_pipe, stderr=err_pipe) Line 30: out, err = p.communicate() Line 31: if p.returncode != 0: Line 32: raise Exception(err) Include rc, out, and err in the exception, we want to see all the available info in case of failures. Line 33: return out Line 34: Line 35: Line 36: def downgrade_vdsm(url): Line 33: return out Line 34: Line 35: Line 36: def downgrade_vdsm(url): Line 37: run_command(['dnf', 'config-manager', '--add-repo', url]) This must also be reverted after the test. Line 38: run_command(['dnf', '--allowerasing', 'install', '--nogpgcheck', Line 39: '-y', 'vdsm-4.17.10.1-0.fc22.noarch']) Line 40: Line 41: Line 35: Line 36: def downgrade_vdsm(url): Line 37: run_command(['dnf', 'config-manager', '--add-repo', url]) Line 38: run_command(['dnf', '--allowerasing', 'install', '--nogpgcheck', Line 39: '-y', 'vdsm-4.17.10.1-0.fc22.noarch']) Why not vdsm-4.17? can work on any platform. Line 40: Line 41: Line 42: def upgrade_vdsm(): Line 43: run_command( Line 48: class UpgradeTest(VdsmTestCase): Line 49: def setUp(self): Line 50: try: Line 51: run_command( Line 52: ['dnf', 'config-manager', '--set-disabled', 'localsync']) This will change the host state after running these tests, bad idea. We want to avoid such global changes that may break other tests. The best would be to save the original value of this configuration, then modify it during the tests, and restore the original value when the tests ends (in tearDown). Line 53: except Exception as e: Line 54: self.fail(e) Line 55: Line 56: def service_up_test(self): Line 50: try: Line 51: run_command( Line 52: ['dnf', 'config-manager', '--set-disabled', 'localsync']) Line 53: except Exception as e: Line 54: self.fail(e) Not needed, setUp will fail without catching and raising again the exception. try here is needed only if you want to cleanup after failures during setup. Line 55: Line 56: def service_up_test(self): Line 57: service_start('vdsmd') Line 58: try: Line 65: self.assertEqual(run_command(['rpm', '-q', 'vdsm']), vdsm_version) Line 66: self.assertEqual(service_status('vdsmd'), 0) Line 67: Line 68: def service_down_test(self): Line 69: service_stop('vdsmd') This assume that vdsm is installed and running - what if the previous test left the host in bad state, vdsm partly installed or not installed? Line 70: try: Line 71: vdsm_version = run_command(['rpm', '-q', 'vdsm']) Line 72: downgrade_vdsm(repo_url) Line 73: upgrade_vdsm() -- To view, visit https://gerrit.ovirt.org/61186 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I589a73fa5285983f7d1adcdae49fc7bffb05bec4 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <[email protected]> Gerrit-Reviewer: Irit Goihman <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
