google / goexpect

Expect for Go
BSD 3-Clause "New" or "Revised" License
759 stars 134 forks source link

Nuking the buffer after a match is not correct IMHO #27

Closed sfilargi closed 5 years ago

sfilargi commented 5 years ago

In ExpectSwitchCase() we do:

            // Clear the buffer directly after match.
            o := tbuf.String()
                        tbuf.Reset()

This means that back to back Expect will not work if the matching string for the second Expect call is already in buffer.

For example the following code will fail:

exp.Expect(regexp.MustComplie("StringA"))
exp.Expect(regexp.MustCopmile("StrinbB"))

If our buffer is already "StringA...StringB" by the time we call exp.Expect(regexp.MustComplie("StringA")).

Real world use case is doing multiple Expect to much long strings during bootup, will work when run against real situation, but will fail during UT.

Instead of nuking the whole buffer, we should only be deleting up to the point of the last match. That's how the rest of "expect" libraries work if I am not mistaken.

skalle commented 5 years ago

Hey sfilargi.

That does make sense and shouldn't be too hard to add in. Might be a good PR for someone. Currently a bit strapped for time so don't know when I personally could get to this so marking it as help wanted for now.

cshung commented 5 years ago

I wanted to attempt this one.

skalle commented 5 years ago

Hey Andrew.

Great, appreciated! .. Happy to help out with any questions you might have.

cshung commented 5 years ago

Hi @skalle, Didn't realize you offered to help until now. I had a busy week.

I stumbled on this issue rather randomly through search - I am neither an experienced go programmer, nor I am a goexpect user. I am hoping to learn more about go through this exercise, which I do.

To my surprise, setting up a repro for this problem seems harder than I wished. I have a feeling that is mostly due to my ignorance. First, I stumbled on #13, which cost me a while to figure out I have to work on Linux. Then I stumbled on #28507 because sudo apt-get install golang give me an old version of golang on Ubuntu. Last but not least, I hit a deadlock in go run process.go 1 in our example. No idea what's wrong with 1, but go run process.go 10 work just fine.

None of the issue above is really goexpect's fault, but it would be really great if people like me don't have to go through that. Maybe we can enhance our README?

Now I think it is time for me to construct an actual repro and start working on it.

skalle commented 5 years ago

Yeah wish I had some cycles to fix #13, already have BSD fixed which I imagine will be quite similar to Mac.

I'm a 100% Linux user myself so might not be of much help setting things up in those environments.

About running the process.

What's done in that little example is it's really calling the linux calculator command bc -l When it bc it calculates PI to whatever number of decimals you specify as an argument.

Let's have a look at what happens when you run it with 1 as an argument.

$ bc -l bc 1.07.1 Copyright 1991-1994, 1997, 1998, 2000, 2004, 2006, 2008, 2012-2017 Free Software Foundation, Inc. This is free software with ABSOLUTELY NO WARRANTY. For details type `warranty'. scale=1 4*a(1) 2.8

So when using a scale of one there it doesn't' look like PI anymore , looking at the code it tries to match the result with.

var piRE = regexp.MustCompile(3.14[0-9]*)

So 2.8 will not match that RE and the program will hang until it hits the Expect timeout. In fact even setting scale to 2 will fail.

scale=2 4*a(1) 3.12

At three we're good again though.

scale=3 4*a(1) 3.140

This is just a small example program so not a big deal but I'd be happy to take a pull request for limiting the arg to >2 ..

As for the README and how to set up an env from scratch, I'll happily accept pull requests. :)

cshung commented 5 years ago

Just an update - I am able to reproduce the issue now.

cshung commented 5 years ago

Shall we close this issue now?

skalle commented 5 years ago

Fixed by Andrew!. Thx.