WIP: Add GUI call functionality to scauto CLI #147
Conversation
|
Please don't mind the unit test failures since unit tests are still a work in progress and they are not reliable yet. I will review the PR ASAP! |
SCAutolib/cli_commands.py
Outdated
| from SCAutolib.models.gui import GUI | ||
| gui = GUI() | ||
| if init: | ||
| gui.__enter__() |
There was a problem hiding this comment.
I think we also need to call exit as well to finish the logging and stop gdm from running.
SCAutolib/cli_commands.py
Outdated
| gui = GUI() | ||
| if init: | ||
| gui.__enter__() | ||
| elif assert_text is not None: |
There was a problem hiding this comment.
Why elif? Don't we want to have a command line "scauto ... gui --init --click-on test --check-home-screen"?
There was a problem hiding this comment.
Mostly because when I started, I wasn't sure how to properly chain things together with the click library. I'll look into this.
There was a problem hiding this comment.
What I was thinking (I don't know if it is possible yet) is to have a list of command objects that will be specified by the arguments and then run the items in that list.
There was a problem hiding this comment.
Ok, I've refactored the code and updated some things that make this run much better now I think. Can you take a look at the recent updates?
SCAutolib/cli_commands.py
Outdated
| default=None, | ||
| show_default=True, | ||
| help="Click on object containing word") | ||
| @click.option("--check-home-screen", |
There was a problem hiding this comment.
Probably we will also need a --check-no-home-screen for negative tests or something.
SCAutolib/cli_commands.py
Outdated
| show_default=True, | ||
| help="Check if screen appears to be the home screen") | ||
| @click.pass_context | ||
| def gui(ctx, init, assert_text, click_on, check_home_screen): |
There was a problem hiding this comment.
In general, I am wondering what is the best approach. Maybe running one thing at a time is not that optimal and it needs a lot of refactoring. Maybe a better approach will be to "construct" a dynamic test through arguments like the tests in https://github.com/redhat-qe-security/SC-tests/blob/main/Graphical/. and then follow somehow the arguments the way they have given. Finally, we can keep the state somehow so as to run multiple commands and initialize it with --init and exit with --done or something. What is your opinion on that?
There was a problem hiding this comment.
I can look into that with the changes requested above I think.
However, I do have use cases for my testing where I'll need to run some commands in between the GUI commands so I'll need to split up some of my GUI calls. But, let me look into click more to find out how to chain the commands together sequentially.
There was a problem hiding this comment.
That is why I thought to have some init and finish flags and when the init one was specified to not run exit or something like that.
SCAutolib/cli_commands.py
Outdated
| @@ -29,7 +32,8 @@ def cli(ctx, force, verbose, conf): | |||
| logger.setLevel(verbose) | |||
| ctx.ensure_object(dict) # Create a dict to store the context | |||
| ctx.obj["FORCE"] = force # Store the force option in the context | |||
| ctx.obj["CONTROLLER"] = Controller(conf) | |||
| if ctx.invoked_subcommand and ctx.invoked_subcommand != "gui": | |||
There was a problem hiding this comment.
Hmm interesting, don't you need some kind of conf to prepare the system? Will you do all the preparation manually?
There was a problem hiding this comment.
Yes, our systems are setup a little differently so we use existing ansible playbooks mostly. So for my specific use case, I don't need to setup anything and thought we might want to be able to run the gui commands without requiring a config file exist for that.
There was a problem hiding this comment.
Ok, fair enough, but we probably need to run some code from the setup for the GUI to work. I will take a look and identify what commands need to be run.
| @@ -290,8 +290,10 @@ def __init__(self, wait_time: float = 5, res_dir_name: str = None): | |||
| # otherwise the first character is not sent | |||
| keyboard.send('enter') | |||
|
|
|||
| def __enter__(self): | |||
| # create screen object to use from calls | |||
| self.screen = Screen(self.screenshot_directory, self.html_file) | |||
There was a problem hiding this comment.
Why this is moved outside of the enter function? I think we need a new fresh screen every time we initialize a test.
There was a problem hiding this comment.
I moved it to the init so it would run outside of context manager use as well. With this change, we should be able to use it either with or without context manager I believe.
|
The other problem that I see is that without initialization of the controller, a lot of required packages and modules that are installed dynamically are not installed. So I guess we can skip configuration (by changing the code) but we need to run at least the setup in the controller (and maybe even cleanup). |
Add CLI GUI commands to make direct calls to the GUI model library functions necessary for some manual and remotely executed test steps. As a part of the change to run GUI functions from the CLI, we also need to move the initialization of the screen object in __init__ instead of __enter__. resolves redhat-qe-security#146
kb_write sent the text directly two keyboard.write() which doesn't handle uppercase characters. A workaround is to use keyboard.sent() to send shift+<char> for the uppercase characters and rely on keyboard.write() to send the rest.
There was a problem hiding this comment.
Sorry for the late review, I was fighting with some other stuff but now it is over. Really great approach, I like it. I have a couple of concerns but it is definitely the right path. I will need to also do more testing with previous tests to see that everything is in order.
| @@ -29,35 +64,11 @@ def cli(ctx, force, verbose, conf): | |||
| logger.setLevel(verbose) | |||
| ctx.ensure_object(dict) # Create a dict to store the context | |||
| ctx.obj["FORCE"] = force # Store the force option in the context | |||
| ctx.obj["CONTROLLER"] = Controller(conf) | |||
| if ctx.invoked_subcommand and not gui: | |||
| ctx.obj["CONTROLLER"] = Controller(check_conf_path(conf)) | |||
There was a problem hiding this comment.
I still think we should have the controller running at least some part of it so the preparation will be done correctly.
|
|
||
| @gui.command() | ||
| @click.argument("name") | ||
| def assert_no_text(name): |
There was a problem hiding this comment.
I guess we can combine them and use --no flag or something like that. Although that way is okay as well.
| gui.__enter__() | ||
| if "assert_text" in action: | ||
| assert_text = action.split(":")[1] | ||
| click.echo(f"CLICK: assert_text:{assert_text}") |
There was a problem hiding this comment.
Do we need the logging? At least I guess it should be bound to debug verbose level.
| if "assert_no_text" in action: | ||
| assert_text = action.split(":")[1] | ||
| click.echo(f"CLICK: assert_text:{assert_text}") | ||
| gui.assert_text(assert_text) |
There was a problem hiding this comment.
Probably assert_no_text here.
| click_on = action.split(":")[1] | ||
| click.echo(f"CLICK: click_on:{click_on}") | ||
| gui.click_on(click_on) | ||
| if "check_home_screen" in action: |
There was a problem hiding this comment.
I thing we need negative for that as well.
| params = action.split(":")[1].split() | ||
| click.echo(f"CLICK: kb_send:{params}") | ||
| gui.kb_send(*params) | ||
| if "kb_write" in action: |
There was a problem hiding this comment.
maybe we can combine them both to write and send, there is no point in only writing with a keyboard I think. And I don't really like this params wildcard. I guess we should put specific params by the flags of the command
| if "init" in action: | ||
| gui.__enter__() | ||
| if "assert_text" in action: | ||
| assert_text = action.split(":")[1] |
There was a problem hiding this comment.
maybe the delimiter is not good in that it is really common and we may need to send it, but maybe not, I need to search it more.
| keyboard.send(f"shift+{char.lower()}") | ||
| else: | ||
| last = f"{last}{char}" | ||
| keyboard.write(*[last], **kwargs) |
There was a problem hiding this comment.
Don't really get the point of this change. Will that work if you send "shift+char"? Doesn't it need a numerical code for the shift to be emulated? I think we have something like that with ctrl.
| @@ -290,8 +290,10 @@ def __init__(self, wait_time: float = 5, res_dir_name: str = None): | |||
| # otherwise the first character is not sent | |||
| keyboard.send('enter') | |||
|
|
|||
| def __enter__(self): | |||
| # create screen object to use from calls | |||
| self.screen = Screen(self.screenshot_directory, self.html_file) | |||
Add CLI GUI commands to make direct calls to the GUI model library functions necessary for some manual and remotely executed test steps.
As a part of the change to run GUI functions from the CLI, we also need to move the initialization of the screen object in init instead of enter.
resolves #146