[1LP][RFR] Fix restore tests third part #9646
Conversation
jarovo
changed the title
jarovo
changed the title
jarovo
changed the title
dajoRH
changed the title
jarovo
changed the title
dajoRH
changed the title
jarovo
changed the title
jarovo
changed the title
There was a problem hiding this comment.
Big PR but I think we have seen most of this stuff before. Nice work @JaryN I have a few required comments but they are fairly trivial.
| @pytest.fixture | ||
| def get_appliance_with_ansible(temp_appliance_preconfig_funcscope): | ||
| """Returns database-owning appliance, enables embedded ansible, | ||
| takes a pg_backup prior to running tests. | ||
|
|
||
| waits for it to ready, takes a backup prior to running tests. |
There was a problem hiding this comment.
What do you mean by waits for it to be ready?
cfme/utils/appliance/__init__.py
Outdated
| """Waits for embedded ansible to be ready | ||
|
|
||
| Args: | ||
| timeout: Number of seconds to wait until timeout (default ``1200``) | ||
| """ | ||
|
|
||
| timeout = 1200 |
There was a problem hiding this comment.
Required I don't think this value should be hard-coded. Why have you removed it as an argument?
There was a problem hiding this comment.
I removed it from the arguments because the value used to be mangled in this function in case the appliance is pod. And I though well this already gets mangled, so I will mangle it bit more but to not make even more confusion I though it would be best to remove the argument.
I guess you are suggesting to put this somewhere as a "constant" right? Perhaps on top of the module. Then do you think this function should even exist? I don't see much of value over just calling the wait_for instead of this.
There was a problem hiding this comment.
I think we should leave the argument in and, if is not specified then we should modify it. I also think the function should exist, as it's useful to have these convenience functions so you don't have to remember exactly what to wait_for. So what I'm suggesting is:
default_timeout = 1200 == timeout
if self.is_pod and default_timeout:
timeout *= 2
if self.version < '5.11' and default_timeout:
timeout *=2If a user passes in something other than the default timeout then we should use that.
There was a problem hiding this comment.
I did it bit differently.
I also decreased the timeout when we are dealing with 5.11 as there I think the ansible should be available almost instantly. What do you thinkg @sbulage ?
There was a problem hiding this comment.
Yes @JaryN on 5.11 version Embedded Ansible gets enabled instantly.
40s seems to be too little
The number got changed for the SCAP. We didn't really do any SCAP.
This does compress the code a bit.
The fixture is redundant. I rather made the tests use the replicated_appliances_with_providers with an argument that is a list of either preupdate appliances or temporary (no-preupdate) appliances depending on which test is requiring them
The were several issues with this test: * There was some leftover code doing reboot. * Reboot was causing the login page to not fully load on 5.11. I think this is some bug that we should investigate, but for now, I want the backup/restore test passing.) * The runcmd was used to check an aws service was running, but the result was not checked. revert
Fixes test_appliance_console_restore_db_external
This helps with test_appliance_console_restore_pg_basebackup_ansible
On 5.11, we seem to be getting the "Embedded ansible" as as "bulit-in provider".
The test is failing (only) on PRT. Waiting for the failover monitor to start using monitoring of the log helps.
Use the generalized waiting method.
The CFME 5.11.2.1 brough re-numbering of the menu options (BZ 1790995). This has to be reflected in the tests code.
jarovo
changed the title
|
I detected some fixture changes in commit 60a6281 The local fixture
The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
jarovo
changed the title
Purpose or Intent
This is my last PR that makes the restore tests fixed.
interaction.answerre.escaperather then definingresc{{py.test: cfme/tests/cli/test_appliance_update.py cfme/tests/cli/test_appliance_console_db_restore.py -v --use-provider 'complete' --collect-logs}}
Remaining unresolved failures on PRT:
test_proxy_*with azure providerstest_appliance_console_restore_pg_basebackup_replicatedoften fails to obtain additional VMs from sprout or is gettingconnection reset by peerwhile in configure()