preserve buffer on reconnect() AND reconnect on connection reset by peer #291
Conversation
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
f65caa1 to
5276f4e
Compare
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
5276f4e to
476afe4
Compare
There was a problem hiding this comment.
Looks good to me, though I'll defer to @MattAitchison on golang usage ;)
adapters/syslog/syslog_test.go
Outdated
| // Read from connection | ||
| go func() { | ||
| defer close(ch) | ||
| defer close(dchan) |
There was a problem hiding this comment.
nitpick
defer func() {
close(ch)
close(chan)
} ()
adapters/syslog/syslog_test.go
Outdated
| done := make(chan bool) | ||
| go func() { | ||
| defer close(logstream) | ||
| defer close(done) |
There was a problem hiding this comment.
similar nitpick on grouping defer into func
| } | ||
| } | ||
|
|
||
| func TestSyslogReconnectOnClose(t *testing.T) { |
There was a problem hiding this comment.
This test is pretty intense.
I'd recommend breaking it down a little. Maybe move the go func() to named functions so they're just go someReallyClearFuncName()
Would make things a little more readable.
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
476afe4 to
67bddd8
Compare
adapters/syslog/syslog_test.go
Outdated
| select { | ||
| case <-done: | ||
| if maxMsgCount-1 != len(datac) { | ||
| t.Errorf("expected %v got %v", maxMsgCount-1, len(datac)) |
There was a problem hiding this comment.
This should check for duplicate messages in addition to msg count.
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
67bddd8 to
bf6dbd3
Compare
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
bf6dbd3 to
43c0db0
Compare
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
3 times, most recently
from
db840d1 to
2c4cf36
Compare
michaelshobbs
force-pushed
the
mh/chore/test-refactor
branch
from
2c4cf36 to
ea5f535
Compare
testutil/mockconn.go
Outdated
|
|
||
| // NewConn returns a new testutil.Conn | ||
| func NewConn() *Conn { | ||
| // A connection consists of two pipes: |
There was a problem hiding this comment.
Since a net.Pipe is a ReadWriter, it represents a duplex channel. I'm not sure you need two of these, which means you probably don't need a lot of the other code you have around these.
testutil/testutil.go
Outdated
| // LocalTCPServer tcp server for testing specific network errors | ||
| type LocalTCPServer struct { | ||
| lnmu sync.RWMutex | ||
| MockConn *Conn |
There was a problem hiding this comment.
The approach of mocking net connections is actually not terribly idiomatic. Because it's relatively easy in Go, most network tests in Go actually stand up a full network client/server instead of building mocks. This reduces a lot of code and better represents reality.
There was a problem hiding this comment.
I agree. If we can force specific connection errors (i.e. timeouts/resets/etc) then we can abandon this approach for sure. I'm playing with some stuff @MattAitchison sent over to see if we can use a "normal" net.Conn object.
There was a problem hiding this comment.
We should be able to. And specific errors are specific cases for specific tests, which can minimally mock what's necessary.
There was a problem hiding this comment.
That's a lot of should happening here. Feel free to chime in with specific pointers.....
michaelshobbs
changed the title
|
@progrium @MattAitchison updated this per our discussion |
|
When is this expected to get pushed up to |
|
This is v3.2.1 and latest now |
make test-directlocally with it on. ref: race: not working with Alpine based image golang/go#14481turns out that in linux (alpine at least) a
conn.Close()shows up as asyscall.ECONNRESET. so i included #287 here as wellcloses #287 #277 #268 #253 #215 #107