Behavior of `Send` is awkward.
Closed this issue · 6 comments
I've found that when I use Send in my code, I have great difficulty achieving the results I would like in terms of tests. I basically have to make a spurious Do call, since nothing else forces a flush of the command queue and expectations apparently aren't checked against that queue.
Would it make sense to have the Flush method actually flush this queue (then modify Do to call the Flush command)? If so, would that be best handled as a default implementation (I.E. something that would not be done if a FlushMock function is provided), or as something that's always performed, regardless of whether a FlushMock is provided?
Or am I perhaps misunderstanding how I should be testing Send commands?
Hey @MrJoy , could you please give an example? From what I tested here it works fine:
issues43.go
package issues43
import "github.com/gomodule/redigo/redis"
func Issues43(conn redis.Conn) error {
if err := conn.Send("MULTI"); err != nil {
return err
}
if err := conn.Send("SET", "person-123", 123456); err != nil {
return err
}
if err := conn.Send("EXPIRE", "person-123", 1000); err != nil {
return err
}
if _, err := conn.Do("EXEC"); err != nil {
return err
}
return nil
}issues43_test.go
package issues43
import (
"testing"
"github.com/rafaeljusto/redigomock"
)
func TestIssues43(t *testing.T) {
conn := redigomock.NewConn()
cmd1 := conn.Command("MULTI")
cmd2 := conn.Command("SET", "person-123", 123456)
cmd3 := conn.Command("EXPIRE", "person-123", 1000)
cmd4 := conn.Command("EXEC").Expect([]interface{}{"OK", "OK"})
if err := Issues43(conn); err != nil {
t.Error(err)
}
if counter := conn.Stats(cmd1); counter != 1 {
t.Errorf("Expected cmd1 to be called once but was called %d times", counter)
}
if counter := conn.Stats(cmd2); counter != 1 {
t.Errorf("Expected cmd2 to be called once but was called %d times", counter)
}
if counter := conn.Stats(cmd3); counter != 1 {
t.Errorf("Expected cmd3 to be called once but was called %d times", counter)
}
if counter := conn.Stats(cmd4); counter != 1 {
t.Errorf("Expected cmd4 to be called once but was called %d times", counter)
}
}This could be related with #15
It works fine if the code under test does a Do at the end. What if it does a Flush instead? Or, what if the responsibility of flushing the queue is left on the caller of the code-under-test?
I can insert a dummy Do in the test itself, with a corresponding Command -- or perhaps just put Receive calls in, but that seems rather awkward. In the case of the former, I have misleading clutter. In the case of the latter I run the risk that I'll get misleading/confusing test failures when the code under test is changed. Specifically, if the number of commands issues by the code-under-test changes, then I might get an error about an expectation not being met (I'm using conn.ExpectationsWereMet()) when the actual problem is that a new command was added and the number of calls to Receive was left unchanged, forcing one of my expected statements to never be flushed from the queue.
Perhaps I'm just thinking about how to structure my code in a weird and unusual way?
Hey @MrJoy , sorry about the delay (was on holidays).
I think that now I understand what you mean:
issues43.go
package issues43
import "github.com/gomodule/redigo/redis"
// Issues43 refers to
// https://github.com/rafaeljusto/redigomock/issues/43#issuecomment-403922314
func Issues43(conn redis.Conn) error {
if err := conn.Send("MULTI"); err != nil {
return err
}
if err := conn.Send("SET", "person-123", 123456); err != nil {
return err
}
if err := conn.Send("EXPIRE", "person-123", 1000); err != nil {
return err
}
return conn.Flush()
}issues43_test.go
package issues43
import (
"testing"
"github.com/rafaeljusto/redigomock"
)
func TestIssues43(t *testing.T) {
conn := redigomock.NewConn()
conn.Command("MULTI")
conn.Command("SET", "person-123", 123456)
conn.Command("EXPIRE", "person-123", 1000)
if err := Issues43(conn); err != nil {
t.Error(err)
}
if err := conn.ExpectationsWereMet(); err != nil {
t.Error(err)
}
}Yeah, this is an issue. I think we could solve this as you suggested:
diff --git a/redigomock.go b/redigomock.go
index e361569..97343f4 100644
--- a/redigomock.go
+++ b/redigomock.go
@@ -144,17 +144,9 @@ func (c *Conn) Clear() {
// response or error is returned. If no registered command is found an error
// is returned
func (c *Conn) Do(commandName string, args ...interface{}) (reply interface{}, err error) {
- // @whazzmaster: Ensures that a call to Do() flushes the command queue
- //
- // The redigo package ensures that a call to Do() will flush any commands
- // that were queued via the Send() method, however a call to Do() on the
- // mock does not empty the queued commands
- for _, cmd := range c.queue {
- if _, err = c.do(cmd.commandName, cmd.args...); err != nil {
- return
- }
+ if err = c.Flush(); err != nil {
+ return
}
- c.queue = []queueElement{}
return c.do(commandName, args...)
}
@@ -210,6 +202,17 @@ func (c *Conn) Send(commandName string, args ...interface{}) error {
// Flush can be mocked using the Conn struct attributes
func (c *Conn) Flush() error {
if c.FlushMock == nil {
+ // @whazzmaster: Ensures that a call to Do() flushes the command queue
+ //
+ // The redigo package ensures that a call to Do() will flush any commands
+ // that were queued via the Send() method, however a call to Do() on the
+ // mock does not empty the queued commands
+ for _, cmd := range c.queue {
+ if _, err := c.do(cmd.commandName, cmd.args...); err != nil {
+ return err
+ }
+ }
+ c.queue = []queueElement{}
return nil
}Just need to think better if this will be a breaking change for those already expecting this dummy flush behaviour.
@rafaeljusto No worries on the delay! Thank you for your patience on this!
Regarding it being a potentially breaking change -- yeah, it seems like it could be. I'd be totally fine with making this an opt-in behavior. I think that has an added benefit of making it clearer in the tests about what behavior is anticipated. I'm not sure how valuable that would be in practice, but I don't think the consequences of it would be terribly disruptive / problematic if it turns out to not be ideal.
Wonderful! Thank you so much!