-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing basic integration testing using Enigo #97
base: master
Are you sure you want to change the base?
Conversation
src/prompts/select.rs
Outdated
self.interact_on_opt(&Term::stderr()) | ||
} | ||
|
||
pub fn set_before(&mut self, f: impl FnMut() + 'a) -> &mut Select<'a> { |
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.
Do we need this fn?
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 might be able to avoid it if I modify and create a custom Term. However, I can promise you it won't be pretty and goes against basic design principles. I agree that we should minimize any fn's that does not benefit the user. So it might be an idea to find an use-case for it?
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.
Could you please explain a bit on why exactly does enigo stuff need to happen after rendering the prompt and not before the whole interact
fn?
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.
It doesn't and I think it should be prior to any rendering. I just did it as a test for the sake of getting visual feedback.
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.
In that case, we don't need this function. Please setup enigo before calling interact in the test.
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.
Next step is:
- We have some test in bottom of prompts/select.rs, do we want that to stay or should it also be moved to tests/select.rs?
- Configure CI so that the integration test passes
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.
Those are unit tests. We don't want to move them to tests
folder.
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.
Got it, I've renamed the pr to integration test (based on this).
Regarding CI, we currently use cargo test --lib
. We have to use cargo test --tests
to also accommodate the integration tests. Seen as I don't have the rights to cancel a workflow, I would rather have you monitoring the workflow with the changes. As it previously created an infinite loop when calling the 'before' fn after the rendering step, but it might still do it (It only happens in the workflow).
Either way, this doesn't have to be rushed by any means, especially given the date, Happy New Year Pavan! 🎆
@pksunkara Even though it doesn't create an endless loop locally, it does on the CI, please cancel this workflow: https://github.com/mitsuhiko/dialoguer/actions/runs/452562230/workflow |
Happy new year to you too. Instead of me monitoring the workflow, you can try to get the CI working in a different branch after enabling github actions for your fork. I think that's a good way to test the CI before updating the PR. |
Is this ready to be reviewed? I didn't get any notification from this PR after my last comment? |
Sorry, have been busy with work. It works locally. However, the integration test sticks in an endless loop when run via CI. I believe this is either a but or a limitation of console which dialoguer is built on top of. You can replicate this issue locally if you use a terminal that hooks into another terminal and runs the program, for example, IntelliJ's in-program terminal has the same issue. I might create an issue over at the console repo. Right now I don't the time nor do I have access to IntelliJ programs as my license has expired. |
Then why do I see green here?
…On Wed, Jan 27, 2021, 17:11 Mikkel Rasmussen ***@***.***> wrote:
Sorry, have been busy with work. It works locally. However, the
integration test sticks in an endless loop when run via CI. I believe this
is either a but or a limitation of console
<https://github.com/mitsuhiko/console> which dialoguer is built on top
of. You can replicate this issue locally if you use a terminal that hooks
into another terminal and runs the program, for example, IntelliJ's
in-program terminal has the same issue. I might create an issue over at the
console repo. Right now I don't the time nor do I have access to IntelliJ
programs as my license has expired.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#97 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABKU35IQJGQ3WRKOBYQ543S4BCKLANCNFSM4VNU5QWQ>
.
|
Because current yaml does not include running the integration test when triggered. So technically you can merge this pr without issues, its just that if you change |
Hello @BlackPhlox, I don't understand why you took a graphical input simulation lib for this.. Terminal programs are pretty simple to test in general because the interface to interact with them is universally known and is based on stream of keys and other escape sequences. To me To do this right @mitsuhiko, I think that the underlying crate |
First draft to implement unit test as referenced in issue #95.
Since now that dialoguer has gotten a CI setup, might be worth the effort.
In order to make integration testing for dialoguer, we have to use a cross-platform input simulation library, the only crate which is capable of this is Enigo. The initial draft has some caveats which are worth reviewing/addressing prior to any merging.
Caveats:
(Not relevant)
Having to extend the API in order to inject keypress simulation.Having to check for initiation of the prompt to prevent an endless loop.Having to use &mut self (prev &self) as the argument on interact (Already made this for input.rs from Allow FnMut for validate #96).Linux Wayland is not supported by Enigo yet.CI Build might fail on Linux if libxdo-dev is not installed (Library for simulating X11 keyboard/mouse input).Result: