Fixing After(duration) to wait during method call #529
Conversation
.gitignore
Outdated
| @@ -22,3 +22,4 @@ _testmain.go | |||
| *.exe | |||
|
|
|||
| .DS_Store | |||
| *.swp | |||
There was a problem hiding this comment.
Can you remove this from the .gitignore?
It is a detail from your editor's configuration. You set a global .gitignore on your machine so Git always ignores .swp files.
mock/mock.go
Outdated
| @@ -52,6 +52,8 @@ type Call struct { | |||
| // receives a message or is closed. nil means it returns immediately. | |||
| WaitFor <-chan time.Time | |||
|
|
|||
| WaitTime time.Duration | |||
There was a problem hiding this comment.
Unexport this, so it has to be set with After.
There was a problem hiding this comment.
Followed thinking its a convention, because other members are exported. will change it.
mock/mock.go
Outdated
| @@ -335,6 +332,7 @@ func (m *Mock) MethodCalled(methodName string, arguments ...interface{}) Argumen | |||
| if call.WaitFor != nil { | |||
| <-call.WaitFor | |||
| } | |||
| time.Sleep(call.WaitTime) | |||
There was a problem hiding this comment.
Rather than doing time.Sleep, do call.WaitFor = time.After(call.waitTime) before the if.
Otherwise it might be possible to set two different waiting times for the same call.
There was a problem hiding this comment.
Then in that case we would be losing the channel which is set via time.WaitUntil (which i can control when to send data not just with timer. Maybe if the channel is nil i could do that, but we might lose the duration in waitTime, Is it a valid case to keep both wait on channel and duration? if not will update it.
There was a problem hiding this comment.
That is not allowed right now anyway, since After currently calls WaitUntil and overrides the property.
The change I mention would be aligned with how things work right now.
By the way, remember to lock/unlock the mutex in After.
There was a problem hiding this comment.
@ernesto-jimenez i've added time.Sleep(waitTime) if waitFor is nil, adding it before would make the duration override, in this case channel is used. Anyway It depends on the client usage WaitUntil(ch).After(d) or .After(d).WaitUntil(ch) and not defined in here. so i hope this is fine.
| return s.Called(i).Get(0).(string) | ||
| } | ||
|
|
||
| func TestAfterTotalWaitTimeWhileExecution(t *testing.T) { |
There was a problem hiding this comment.
These tests slow down the test run by ˜3 seconds.
Can you reduce wait times a lot more to keep tests quick?
mock/mock_test.go
Outdated
| } | ||
|
|
||
| func TestAfterTotalWaitTimeWhileExecution(t *testing.T) { | ||
| waitDuration := 10 |
There was a problem hiding this comment.
would 1ms be enough here?
The faster we can make it, the better :)
|
@Dineshkumar-cse awesome! do you mind squashing into a single commit? Thanks |
commit 5b0291d47dc3a70cc6c6be3bba3c2f934a8e933e Merge: 1f324ec 8ccf48a Author: Dinesh Kumar <dineshkumar-cse@users.noreply.github.com> Date: Sat Dec 30 19:55:13 2017 +0530 Merge branch 'master' into master commit 1f324ec Author: Dinesh Kumar <dinesh.kumar@go-jek.com> Date: Sat Dec 30 19:07:16 2017 +0530 Fixing comments: reduced test time, locking in after, unexported waitTime - WaitUntil/After overrides each other value - currently if channel is set that takes the priority, if not the waitTime is used to Sleep commit a7101ec Author: Dinesh Kumar <dinesh.kumar@go-jek.com> Date: Fri Dec 29 13:02:14 2017 +0530 Using if else instead of switch and pulling out the commone one commit 936f63d Author: Dinesh Kumar <dinesh.kumar@go-jek.com> Date: Fri Dec 29 12:41:40 2017 +0530 After - making it wait during method call After was using call.WaitUntil(time.After(duration)), <-time.After(duration) returns a channel with the timer immediately started
|
Done. @ernesto-jimenez Thanks for reviewing it. We can close #464 |
|
Cool, will merge once CI passes. Thanks! |
After was using call.WaitUntil(time.After(duration)),
ch := time.After(duration)returns a channel with the timer immediately started.