-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] CLI-Testing UI after restarting VMDB #9864
[1LP][RFR] CLI-Testing UI after restarting VMDB #9864
Conversation
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.
LGTM :)
c6ac82e
to
835a548
Compare
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.
Some comment. please have glance.
Resetting database means we can expect data will be loss. Its OK but still i would prefer to use temp appliance.
|
||
command_set = ("ap", RETURN, "7", "4", "Y", TimedCommand("99", 300)) | ||
result = appliance.appliance_console.run_commands(command_set, timeout=30) | ||
assert "Database reset successfully" in result[-1], "DB reset failed" |
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.
optional: I think we should work on result class for appliance_console
which will provide output
dict
of command and output.
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.
@digitronik , Thank your suggestion but if we make output dict
instead of list
then here we are going to override result.
Example:
commands = ("ap", "", "5", "2", "1", "2", "2")
In above example we will not able to store result separately for all 2 and 1 command and we will not get which command executed first due to its dict
type.
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.
Using SSHExpect is a better option here, as its asserting along the way what the prompts are.
Further asserting the output content, we'll have to deal with the list of returned stdout strings.
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.
@mshriver , The code which I used I think that is more simple than SSHExpect due to below reason
- Expected stdout is not the same for every command so have to define separate SSHExpect.answer() for each call instead for/while loop so code will become more lengthy
- Hard to manage timeout than current code
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.
I like that you are doing something about making the answers better defined than just having a stream of numbers. Bad maintainability in case of renumbering was one of the reasons why I started with the SSHExpect thing. I though was aiming even higher -- defining something like a "menu tree model", but didn't get into finishing this.
I think it may be good to unify our approaches in one thing if possible. For this I would like need you to help me understand what should be improved.
About the expectations on stdout... well the question here is whether you want to check what menu question are you answering. Of course when you start checking more things, the test will produce negative (FAIL) results more often. This is not a bad thing every time. I think if the test should fail, it should fail as soon as possible. I think this improves its maintainability. The cost is that even slight change (like a typo correction or such) may cause test to fail while I would say this will be false negative. But I think the false negatives may be eliminated to a good extend a lot by having smart checks. I am not checking all the menu, but the last thing in the SSHExpect buffer (which is actually not too long, especially on PRT, and one should know about it.), which is I think every time, the relevant menu question.
About the timeout, I don't understand what is the problem
with SSHExpect(appl) as interaction:
interaction.answer('Press any key to continue.', '', timeout=20)
This will wait 20s for the prompt and when it sees it it will send an empty string that is AFAIR followed by some newline character.
interaction.answer('Enter the cluster database password: ', pwd)
will wait default timeout and then send pwd. I don't see what is hard here.
835a548
to
c97b057
Compare
@digitronik , incorporated the review comments, please have a look once |
command_set = ("ap", RETURN, "17", TimedCommand("Y", 120)) | ||
appliance.appliance_console.run_commands(command_set, timeout=30) | ||
|
||
wait_for(lambda: appliance.wait_for_web_ui(), delay=10, timeout=300) |
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.
No need to wrap this in a wait_for (or to use lambda if wait_for was necessary).
The function is already waiting, and will raise exception.
https://github.com/ManageIQ/integration_tests/blob/master/cfme/utils/appliance/__init__.py#L1490
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.
Done
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.
Please use wait_for_web_ui
directly.
c97b057
to
c66171b
Compare
c66171b
to
d7372ee
Compare
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.
Thanks for menu mapping 👍 Nice PR
It will usefull as options are changed.
Purpose or Intent
adding_test Testing the appliance UI after restarting the appliance database.
PRT Run
{{pytest: ./cfme/tests/cli/test_appliance_console.py::test_appliance_console_vmdb_httpd -v}}