Parallelize TestEventsLimit#12719
Parallelize TestEventsLimit#12719cpuguy83 merged 1 commit intomoby:masterfrom fntlnz:12675-test-events-emit
Conversation
There was a problem hiding this comment.
I'm afraid there's a race there: we're starting 30 goroutines but immediately call for docker events, I don't think we have any guarantee to reliability get the 64 expected events in the end.
I'd add a sync.WaitGroup: @LK4D4 what do you think?
There was a problem hiding this comment.
Also can't use dockerCmd in a goroutine like that since it calls c.Fatal on error.
There was a problem hiding this comment.
Better to use directly runCommandWithOutput ?
There was a problem hiding this comment.
Yes, and any error would need to be dealt with from the main goroutine.
There was a problem hiding this comment.
Better synchronize everything with a channel to handle eventual errors from dockerCmd runCommandWithOutput and I'd use another to handle docker events streaming checks
There was a problem hiding this comment.
Thank you for your hints :)
There was a problem hiding this comment.
should be really buferized to 30, to let goroutines just die.
|
@LK4D4 looks good to you? |
There was a problem hiding this comment.
nit, I'd call it errChan since errors could be imported (again, nit, don't necessary do it and maintainers have last word of course, wait for them and in case make this change)
There was a problem hiding this comment.
Yeah, errors is package from stdlib, so, can be confusing.
There was a problem hiding this comment.
Yeah, thank you, I'm fixing this
Signed-off-by: Lorenzo Fontana <fontanalorenzo@me.com>
|
For me test 4 times faster. |
|
LGTM |
fix #12675
Signed-off-by: Lorenzo Fontana fontanalorenzo@me.com