Respect timeout on Send command use

cynepco3hahue commented 5 years ago

Before this PR, Send stuck forever if the spawned process does not ready to get input.

Fixes #30

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

cynepco3hahue commented 5 years ago

I signed it!

googlebot commented 5 years ago

CLAs look good, thanks!

cynepco3hahue commented 5 years ago

@skalle Give me know if I missed something.

skalle commented 5 years ago

Hey @cynepco3hahue.

We probably don't want to change the Send command itself, the API is pretty set.

There's a few options to handle adding a timeout to Send.

Add another Send func with timeout eg. SendWithTimeout.
Add an option to set the Send timeout.
- This would apply to all Sends though so seem less flexible compared to the first option.
- Would be the easiest change with no changes to any interfaces.
Somehow use the expect timeout.
- Would probably just be confusing.

What about combining 1 & 2 , add an option to set the default Send timeout , this would apply to the "old" version of Send too.. If not set it could be infinity as it is today. On top of that add another SendWithTimeout method where you can set the desired timeout.

skalle commented 5 years ago

On some more thinking ..

With Send being pretty much only a Write, I don't think the need for a timeout in the method would be used much. Having an option to set a timeout would probably be enough. Option 2 that is.

cynepco3hahue commented 5 years ago

@skalle Thanks for detailed explanation, I refactored the code change to use option 2, maybe do you have an idea how can I test it with current a SpawnFake method?

skalle commented 5 years ago

Sorry for the delay here, just not enough cycles atm.

PR looks good , just need a test in there. As often happens the tests might be harder than the added feature. Should be fairly straight forward to setup a SpawnGeneric where the writer function , instead of the usual `go io.Copy(..pipes..) would be a writer function that delays the write for some specified time..

Thx. contributing and happy to help out if you get stuck with the tests.

cynepco3hahue commented 5 years ago

@skalle Sure no problem, we all have a lot of tasks :smile: I added unittest, can you please review again?

skalle commented 5 years ago

You added that in faster than I expected .. Some nits in there, with that sorted I'll merge it in..

Thx!

skalle commented 5 years ago

Merged in .. Thx. a lot for making this project better!

cynepco3hahue commented 5 years ago

@skalle Great, thanks for your help with review:)

google / goexpect