Add Reset and PrepareForAssert methods #16
Conversation
This enables tests for code that uses TritonD client in async way to synchronize on number of messages written.
|
I created an issue to address the problem in general: #17 |
tritond/mock.go
Outdated
| c.lock.Unlock() | ||
| //Code that uses TritonD client in async way might want to able to block till Put is finished for testing | ||
| go func() { c.Channel <- data }() | ||
| c.Lock.Unlock() |
There was a problem hiding this comment.
I know this isn't your code, but is there any reason why this isn't defer c.Lock.Unlock and moved to right after the c.Lock.Lock() statement?
We need reset method as quite often test clients are created once for test package and the same client is reused across multiple tests. It might be handy to reset client so net effect of one test can be evaluated. In async usages of TritonD client we need to make sure we yield to goroutines so that logging is actually complete and that we synchronize assertion code so that race detector doesn't complain about it.
We need reset method as quite often test clients are created once for test package and the same client is reused across multiple tests. It might be handy to reset client so net effect of one test can be evaluated. In async usages of TritonD client we need to make sure we yield to goroutines so that logging is actually complete and that we synchronize assertion code so that race detector doesn't complain about it.
|
@cvanderschuere Do you know why circle is failing for this branch? |
mikhailkolesnik
changed the title
| @@ -37,6 +38,26 @@ func (c *MockClient) Close(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Reset resets MockClient to the initial state | |||
| func (c *MockClient) Reset() { | |||
| c.lock.Lock() | |||
There was a problem hiding this comment.
won't this block if the lock is held by another, so a call to Reset could result in a deadlock?
There was a problem hiding this comment.
Still thinking this will block if another context has the client holding the lock.
There was a problem hiding this comment.
Since we have only one lock - it should not cause any deadlock. As who ever holds it should eventually let it go.
mikhailkolesnik
force-pushed
the
mkolesnik/MockClient_Async
branch
from
f2e6dca
to
8f545c0
Compare
tritond/main_test.go
Outdated
| @@ -80,11 +81,11 @@ func (c *consumer) Start() error { | |||
| } | |||
| } | |||
| }() | |||
| runtime.Gosched() | |||
There was a problem hiding this comment.
We should try and avoid this if possible and It's not needed in this case
tritond/client_test.go
Outdated
| @@ -21,7 +21,7 @@ func TestPut(t *testing.T) { | |||
| data := map[string]interface{}{ | |||
| "object_type": "delivery", | |||
| "delivery_uuid": "example-delivery-uuid", | |||
| "ts": time.Now(), | |||
| "ts": time.Now().Round(0), | |||
tritond/main_test.go
Outdated
| } | ||
|
|
||
| if err = c.socket.Bind("tcp://127.0.0.1:3515"); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
This error is much harder to deal with than the previous serial version.
There was a problem hiding this comment.
why not just return the err on a channel (or put it in an error slice) and wait for all go routines to finish then check the error and return it?
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *consumer) Stop() error { | ||
| func (c *consumer) Stop() { | ||
| close(c.close) |
There was a problem hiding this comment.
This can also be solved by requiring callers to do this on the same goroutine right?
There was a problem hiding this comment.
No. The problem is that the read happens on the different one. So close must happen on the same one that the read and create happens on.
There was a problem hiding this comment.
Realized this is only for the test client so this is much less important
| func (c *MockClient) PrepareForAssert() func() { | ||
| runtime.Gosched() | ||
| c.lock.Lock() | ||
| return c.unlock |
There was a problem hiding this comment.
Personal preference would be to use an anonymous function here so it can be very specific to this use case.
There was a problem hiding this comment.
Also, not sure if you really want runtime.Gosched(). It looks like that forces a context switch, which might not be what you want?
https://stackoverflow.com/questions/13107958/what-exactly-does-runtime-gosched-do
There was a problem hiding this comment.
The idea is to make it reusable for all cases when clients want to test async usages of this code. Anonymous func in my code is not reusable.
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *consumer) Stop() error { | ||
| func (c *consumer) Stop() { | ||
| close(c.close) |
There was a problem hiding this comment.
Realized this is only for the test client so this is much less important
|
|
||
| if err = c.socket.Bind("tcp://127.0.0.1:3515"); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
A bit of a weird pattern, but works
This enables tests for code that uses TritonD client in async way
to synchronize on number of messages written.