From c481300217869ed224fbfc92a8d84ff389e44c68 Mon Sep 17 00:00:00 2001 From: Alex Bramley Date: Sat, 16 Feb 2013 00:11:39 +0000 Subject: [PATCH] Remove embedded logger from client package. Conflicts: client/connection.go --- client/connection.go | 37 +++++++--------- client/connection_test.go | 26 +++-------- client/handlers_test.go | 90 ++++++++++++--------------------------- client/state_handlers.go | 21 ++++----- 4 files changed, 59 insertions(+), 115 deletions(-) diff --git a/client/connection.go b/client/connection.go index 7cbfa21..0deca91 100644 --- a/client/connection.go +++ b/client/connection.go @@ -32,9 +32,6 @@ type Conn struct { ST state.StateTracker st bool - // Logger for debugging/warning/etc output - l logging.Logger - // Use the State field to store external state that handlers might need. // Remember ... you might need locking for this ;-) State interface{} @@ -69,7 +66,6 @@ type Conn struct { // that you can add event handlers to it. See AddHandler() for details. func SimpleClient(nick string, args ...string) *Conn { r := event.NewRegistry() - l := logging.InitFromFlags() ident := "goirc" name := "Powered by GoIRC" @@ -79,18 +75,17 @@ func SimpleClient(nick string, args ...string) *Conn { if len(args) > 1 && args[1] != "" { name = args[1] } - return Client(nick, ident, name, r, l) + return Client(nick, ident, name, r) } -func Client(nick, ident, name string, - r event.EventRegistry, l logging.Logger) *Conn { - if r == nil || l == nil { +func Client(nick, ident, name string, r event.EventRegistry) *Conn { + if r == nil { return nil } + logging.InitFromFlags() conn := &Conn{ ER: r, ED: r, - l: l, st: false, in: make(chan *Line, 32), out: make(chan string, 32), @@ -106,7 +101,7 @@ func Client(nick, ident, name string, lastsent: time.Now(), } conn.addIntHandlers() - conn.Me = state.NewNick(nick, l) + conn.Me = state.NewNick(nick) conn.Me.Ident = ident conn.Me.Name = name @@ -117,7 +112,7 @@ func Client(nick, ident, name string, func (conn *Conn) EnableStateTracking() { if !conn.st { n := conn.Me - conn.ST = state.NewTracker(n.Nick, conn.l) + conn.ST = state.NewTracker(n.Nick) conn.Me = conn.ST.Me() conn.Me.Ident = n.Ident conn.Me.Name = n.Name @@ -160,7 +155,7 @@ func (conn *Conn) Connect(host string, pass ...string) error { if !hasPort(host) { host += ":6697" } - conn.l.Info("irc.Connect(): Connecting to %s with SSL.", host) + logging.Info("irc.Connect(): Connecting to %s with SSL.", host) if s, err := tls.Dial("tcp", host, conn.SSLConfig); err == nil { conn.sock = s } else { @@ -170,7 +165,7 @@ func (conn *Conn) Connect(host string, pass ...string) error { if !hasPort(host) { host += ":6667" } - conn.l.Info("irc.Connect(): Connecting to %s without SSL.", host) + logging.Info("irc.Connect(): Connecting to %s without SSL.", host) if s, err := net.Dial("tcp", host); err == nil { conn.sock = s } else { @@ -226,18 +221,18 @@ func (conn *Conn) recv() { for { s, err := conn.io.ReadString('\n') if err != nil { - conn.l.Error("irc.recv(): %s", err.Error()) + logging.Error("irc.recv(): %s", err.Error()) conn.shutdown() return } s = strings.Trim(s, "\r\n") - conn.l.Debug("<- %s", s) + logging.Debug("<- %s", s) if line := parseLine(s); line != nil { line.Time = time.Now() conn.in <- line } else { - conn.l.Warn("irc.recv(): problems parsing line:\n %s", s) + logging.Warn("irc.recv(): problems parsing line:\n %s", s) } } } @@ -275,23 +270,23 @@ func (conn *Conn) write(line string) { if !conn.Flood { if t := conn.rateLimit(len(line)); t != 0 { // sleep for the current line's time value before sending it - conn.l.Debug("irc.rateLimit(): Flood! Sleeping for %.2f secs.", + logging.Debug("irc.rateLimit(): Flood! Sleeping for %.2f secs.", t.Seconds()) <-time.After(t) } } if _, err := conn.io.WriteString(line + "\r\n"); err != nil { - conn.l.Error("irc.send(): %s", err.Error()) + logging.Error("irc.send(): %s", err.Error()) conn.shutdown() return } if err := conn.io.Flush(); err != nil { - conn.l.Error("irc.send(): %s", err.Error()) + logging.Error("irc.send(): %s", err.Error()) conn.shutdown() return } - conn.l.Debug("-> %s", line) + logging.Debug("-> %s", line) } // Implement Hybrid's flood control algorithm to rate-limit outgoing lines. @@ -317,7 +312,7 @@ func (conn *Conn) shutdown() { // Guard against double-call of shutdown() if we get an error in send() // as calling sock.Close() will cause recv() to recieve EOF in readstring() if conn.Connected { - conn.l.Info("irc.shutdown(): Disconnected from server.") + logging.Info("irc.shutdown(): Disconnected from server.") conn.ED.Dispatch(DISCONNECTED, conn, &Line{}) conn.Connected = false conn.sock.Close() diff --git a/client/connection_test.go b/client/connection_test.go index caf5812..f6cb1ff 100644 --- a/client/connection_test.go +++ b/client/connection_test.go @@ -13,7 +13,6 @@ import ( type testState struct { ctrl *gomock.Controller - log *logging.MockLogger st *state.MockStateTracker ed *event.MockEventDispatcher nc *mockNetConn @@ -25,13 +24,9 @@ func setUp(t *testing.T, start ...bool) (*Conn, *testState) { st := state.NewMockStateTracker(ctrl) r := event.NewRegistry() ed := event.NewMockEventDispatcher(ctrl) - l := logging.NewMockLogger(ctrl) nc := MockNetConn(t) - c := Client("test", "test", "Testing IRC", r, l) - - // We don't want to have to specify s.log.EXPECT().Debug() for all the - // random crap that gets logged. This mocks it all out nicely. - ctrl.RecordCall(l, "Debug", gomock.Any(), gomock.Any()).AnyTimes() + c := Client("test", "test", "Testing IRC", r) + logging.SetLogLevel(logging.LogFatal) c.ED = ed c.ST = st @@ -47,14 +42,12 @@ func setUp(t *testing.T, start ...bool) (*Conn, *testState) { <-time.After(1e6) } - return c, &testState{ctrl, l, st, ed, nc, c} + return c, &testState{ctrl, st, ed, nc, c} } func (s *testState) tearDown() { s.ed.EXPECT().Dispatch("disconnected", s.c, &Line{}) s.st.EXPECT().Wipe() - s.log.EXPECT().Error("irc.recv(): %s", "EOF") - s.log.EXPECT().Info("irc.shutdown(): Disconnected from server.") s.nc.ExpectNothing() s.c.shutdown() <-time.After(1e6) @@ -71,8 +64,6 @@ func TestEOF(t *testing.T) { // Simulate EOF from server s.ed.EXPECT().Dispatch("disconnected", c, &Line{}) s.st.EXPECT().Wipe() - s.log.EXPECT().Info("irc.shutdown(): Disconnected from server.") - s.log.EXPECT().Error("irc.recv(): %s", "EOF") s.nc.Close() // Since things happen in different internal goroutines, we need to wait @@ -89,7 +80,6 @@ func TestClientAndStateTracking(t *testing.T) { // This doesn't use setUp() as we want to pass in a mock EventRegistry. ctrl := gomock.NewController(t) r := event.NewMockEventRegistry(ctrl) - l := logging.NewMockLogger(ctrl) st := state.NewMockStateTracker(ctrl) for n, _ := range intHandlers { @@ -99,10 +89,10 @@ func TestClientAndStateTracking(t *testing.T) { // handler names are correctly passed to AddHandler. ctrl.RecordCall(r, "AddHandler", gomock.Any(), n) } - c := Client("test", "test", "Testing IRC", r, l) + c := Client("test", "test", "Testing IRC", r) // Assert some basic things about the initial state of the Conn struct - if c.ER != r || c.ED != r || c.l != l || c.st != false || c.ST != nil { + if c.ER != r || c.ED != r || c.st != false || c.ST != nil { t.Errorf("Conn not correctly initialised with external deps.") } if c.in == nil || c.out == nil || c.cSend == nil || c.cLoop == nil { @@ -246,8 +236,6 @@ func TestRecv(t *testing.T) { // Test that recv does something useful with a line it can't parse // (not that there are many, parseLine is forgiving). - s.log.EXPECT().Warn("irc.recv(): problems parsing line:\n %s", - ":textwithnospaces") s.nc.Send(":textwithnospaces") if l := reader(); l != nil { t.Errorf("Bad line still caused receive on input channel.") @@ -259,8 +247,6 @@ func TestRecv(t *testing.T) { } s.ed.EXPECT().Dispatch("disconnected", c, &Line{}) s.st.EXPECT().Wipe() - s.log.EXPECT().Info("irc.shutdown(): Disconnected from server.") - s.log.EXPECT().Error("irc.recv(): %s", "EOF") s.nc.Close() // Since send and runloop aren't actually running, we need to empty their @@ -449,8 +435,6 @@ func TestWrite(t *testing.T) { s.ed.EXPECT().Dispatch("disconnected", c, &Line{}) s.st.EXPECT().Wipe() - s.log.EXPECT().Info("irc.shutdown(): Disconnected from server.") - s.log.EXPECT().Error("irc.send(): %s", "invalid argument") c.write("she can't pass unit tests") } diff --git a/client/handlers_test.go b/client/handlers_test.go index 8fdfef8..e622da0 100644 --- a/client/handlers_test.go +++ b/client/handlers_test.go @@ -158,7 +158,7 @@ func TestJOIN(t *testing.T) { defer s.tearDown() // The state tracker should be creating a new channel in this first test - chan1 := state.NewChannel("#test1", s.log) + chan1 := state.NewChannel("#test1") gomock.InOrder( s.st.EXPECT().GetChannel("#test1").Return(nil), @@ -176,7 +176,7 @@ func TestJOIN(t *testing.T) { s.nc.Expect("WHO #test1") // In this second test, we should be creating a new nick - nick1 := state.NewNick("user1", s.log) + nick1 := state.NewNick("user1") gomock.InOrder( s.st.EXPECT().GetChannel("#test1").Return(chan1), @@ -192,7 +192,7 @@ func TestJOIN(t *testing.T) { s.nc.Expect("WHO user1") // In this third test, we'll be pretending we know about the nick already. - nick2 := state.NewNick("user2", c.l) + nick2 := state.NewNick("user2") gomock.InOrder( s.st.EXPECT().GetChannel("#test1").Return(chan1), s.st.EXPECT().GetNick("user2").Return(nick2), @@ -205,13 +205,9 @@ func TestJOIN(t *testing.T) { // unknown channel, unknown nick s.st.EXPECT().GetChannel("#test2").Return(nil), s.st.EXPECT().GetNick("blah").Return(nil), - s.log.EXPECT().Warn("irc.JOIN(): JOIN to unknown channel %s "+ - "received from (non-me) nick %s", "#test2", "blah"), // unknown channel, known nick that isn't Me. s.st.EXPECT().GetChannel("#test2").Return(nil), s.st.EXPECT().GetNick("user2").Return(nick2), - s.log.EXPECT().Warn("irc.JOIN(): JOIN to unknown channel %s "+ - "received from (non-me) nick %s", "#test2", "user2"), ) c.h_JOIN(parseLine(":blah!moo@cows.com JOIN :#test2")) c.h_JOIN(parseLine(":user2!ident2@host2.com JOIN :#test2")) @@ -223,8 +219,8 @@ func TestPART(t *testing.T) { defer s.tearDown() // We need some valid and associated nicks / channels to PART with. - chan1 := state.NewChannel("#test1", s.log) - nick1 := state.NewNick("user1", s.log) + chan1 := state.NewChannel("#test1") + nick1 := state.NewNick("user1") // PART should dissociate a nick from a channel. gomock.InOrder( @@ -242,8 +238,8 @@ func TestKICK(t *testing.T) { defer s.tearDown() // We need some valid and associated nicks / channels to KICK. - chan1 := state.NewChannel("#test1", s.log) - nick1 := state.NewNick("user1", s.log) + chan1 := state.NewChannel("#test1") + nick1 := state.NewNick("user1") // KICK should dissociate a nick from a channel. gomock.InOrder( @@ -269,8 +265,8 @@ func TestMODE(t *testing.T) { c, s := setUp(t) defer s.tearDown() - chan1 := state.NewChannel("#test1", s.log) - nick1 := state.NewNick("user1", s.log) + chan1 := state.NewChannel("#test1") + nick1 := state.NewNick("user1") // Send a channel mode line. Inconveniently, Channel and Nick objects // aren't mockable with gomock as they're not interface types (and I @@ -298,13 +294,9 @@ func TestMODE(t *testing.T) { // send a valid user mode that's not us s.st.EXPECT().GetChannel("user1").Return(nil), s.st.EXPECT().GetNick("user1").Return(nick1), - s.log.EXPECT().Warn("irc.MODE(): recieved MODE %s for (non-me) nick %s", - "+w", "user1"), // Send a random mode for an unknown channel s.st.EXPECT().GetChannel("#test2").Return(nil), s.st.EXPECT().GetNick("#test2").Return(nil), - s.log.EXPECT().Warn("irc.MODE(): not sure what to do with MODE %s", - "#test2 +is"), ) c.h_MODE(parseLine(":user1!ident1@host1.com MODE user1 +w")) c.h_MODE(parseLine(":user1!ident1@host1.com MODE #test2 +is")) @@ -315,7 +307,7 @@ func TestTOPIC(t *testing.T) { c, s := setUp(t) defer s.tearDown() - chan1 := state.NewChannel("#test1", s.log) + chan1 := state.NewChannel("#test1") // Assert that it has no topic originally if chan1.Topic != "" { @@ -332,11 +324,7 @@ func TestTOPIC(t *testing.T) { } // Check error paths -- send a topic for an unknown channel - gomock.InOrder( - s.st.EXPECT().GetChannel("#test2").Return(nil), - s.log.EXPECT().Warn("irc.TOPIC(): topic change on unknown channel %s", - "#test2"), - ) + s.st.EXPECT().GetChannel("#test2").Return(nil) c.h_TOPIC(parseLine(":user1!ident1@host1.com TOPIC #test2 :dark side")) } @@ -346,7 +334,7 @@ func Test311(t *testing.T) { defer s.tearDown() // Create user1, who we know little about - nick1 := state.NewNick("user1", s.log) + nick1 := state.NewNick("user1") // Send a 311 reply s.st.EXPECT().GetNick("user1").Return(nick1) @@ -360,11 +348,7 @@ func Test311(t *testing.T) { } // Check error paths -- send a 311 for an unknown nick - gomock.InOrder( - s.st.EXPECT().GetNick("user2").Return(nil), - s.log.EXPECT().Warn("irc.311(): received WHOIS info for unknown nick %s", - "user2"), - ) + s.st.EXPECT().GetNick("user2").Return(nil) c.h_311(parseLine(":irc.server.org 311 test user2 ident2 host2.com * :dongs")) } @@ -374,7 +358,7 @@ func Test324(t *testing.T) { defer s.tearDown() // Create #test1, whose modes we don't know - chan1 := state.NewChannel("#test1", s.log) + chan1 := state.NewChannel("#test1") // Send a 324 reply s.st.EXPECT().GetChannel("#test1").Return(chan1) @@ -383,12 +367,8 @@ func Test324(t *testing.T) { t.Errorf("Channel.ParseModes() not called correctly.") } - // Check unknown channel causes an error - gomock.InOrder( - s.st.EXPECT().GetChannel("#test2").Return(nil), - s.log.EXPECT().Warn("irc.324(): received MODE settings for unknown "+ - "channel %s", "#test2"), - ) + // Check error paths -- send 324 for an unknown channel + s.st.EXPECT().GetChannel("#test2").Return(nil) c.h_324(parseLine(":irc.server.org 324 test #test2 +pmt")) } @@ -398,7 +378,7 @@ func Test332(t *testing.T) { defer s.tearDown() // Create #test1, whose topic we don't know - chan1 := state.NewChannel("#test1", s.log) + chan1 := state.NewChannel("#test1") // Assert that it has no topic originally if chan1.Topic != "" { @@ -414,12 +394,8 @@ func Test332(t *testing.T) { t.Errorf("Topic of test channel not set correctly.") } - // Check unknown channel causes an error - gomock.InOrder( - s.st.EXPECT().GetChannel("#test2").Return(nil), - s.log.EXPECT().Warn("irc.332(): received TOPIC value for unknown "+ - "channel %s", "#test2"), - ) + // Check error paths -- send 332 for an unknown channel + s.st.EXPECT().GetChannel("#test2").Return(nil) c.h_332(parseLine(":irc.server.org 332 test #test2 :dark side")) } @@ -429,7 +405,7 @@ func Test352(t *testing.T) { defer s.tearDown() // Create user1, who we know little about - nick1 := state.NewNick("user1", s.log) + nick1 := state.NewNick("user1") // Send a 352 reply s.st.EXPECT().GetNick("user1").Return(nick1) @@ -453,11 +429,7 @@ func Test352(t *testing.T) { } // Check error paths -- send a 352 for an unknown nick - gomock.InOrder( - s.st.EXPECT().GetNick("user2").Return(nil), - s.log.EXPECT().Warn("irc.352(): received WHO reply for unknown nick %s", - "user2"), - ) + s.st.EXPECT().GetNick("user2").Return(nil) c.h_352(parseLine(":irc.server.org 352 test #test2 ident2 host2.com irc.server.org user2 G :0 fooo")) } @@ -467,7 +439,7 @@ func Test353(t *testing.T) { defer s.tearDown() // Create #test1, whose user list we're mostly unfamiliar with - chan1 := state.NewChannel("#test1", s.log) + chan1 := state.NewChannel("#test1") // Create maps for testing -- this is what the mock ST calls will return nicks := make(map[string]*state.Nick) @@ -478,7 +450,7 @@ func Test353(t *testing.T) { for _, n := range []string{"user1", "user2", "voice", "halfop", "op", "admin", "owner"} { - nicks[n] = state.NewNick(n, s.log) + nicks[n] = state.NewNick(n) privs[n] = new(state.ChanPrivs) } @@ -515,12 +487,8 @@ func Test353(t *testing.T) { t.Errorf("353 handler failed to set correct modes for nicks.") } - // Check unknown channel causes a warning - gomock.InOrder( - s.st.EXPECT().GetChannel("#test2").Return(nil), - s.log.EXPECT().Warn("irc.353(): received NAMES list for unknown "+ - "channel %s", "#test2"), - ) + // Check error paths -- send 353 for an unknown channel + s.st.EXPECT().GetChannel("#test2").Return(nil) c.h_353(parseLine(":irc.server.org 353 test = #test2 :test ~user3")) } @@ -530,7 +498,7 @@ func Test671(t *testing.T) { defer s.tearDown() // Create user1, who should not be secure - nick1 := state.NewNick("user1", s.log) + nick1 := state.NewNick("user1") if nick1.Modes.SSL { t.Errorf("Test nick user1 is already using SSL?") } @@ -545,10 +513,6 @@ func Test671(t *testing.T) { } // Check error paths -- send a 671 for an unknown nick - gomock.InOrder( - s.st.EXPECT().GetNick("user2").Return(nil), - s.log.EXPECT().Warn("irc.671(): received WHOIS SSL info for unknown "+ - "nick %s", "user2"), - ) + s.st.EXPECT().GetNick("user2").Return(nil) c.h_671(parseLine(":irc.server.org 671 test user2 :some ignored text")) } diff --git a/client/state_handlers.go b/client/state_handlers.go index efd8c48..34e87ee 100644 --- a/client/state_handlers.go +++ b/client/state_handlers.go @@ -5,6 +5,7 @@ package client import ( "github.com/fluffle/goevent/event" + "github.com/fluffle/golog/logging" "strings" ) @@ -53,7 +54,7 @@ func (conn *Conn) h_JOIN(line *Line) { // first we've seen of this channel, so should be us joining it // NOTE this will also take care of nk == nil && ch == nil if nk != conn.Me { - conn.l.Warn("irc.JOIN(): JOIN to unknown channel %s received "+ + logging.Warn("irc.JOIN(): JOIN to unknown channel %s received "+ "from (non-me) nick %s", line.Args[0], line.Nick) return } @@ -105,13 +106,13 @@ func (conn *Conn) h_MODE(line *Line) { } else if nk := conn.ST.GetNick(line.Args[0]); nk != nil { // nick mode change, should be us if nk != conn.Me { - conn.l.Warn("irc.MODE(): recieved MODE %s for (non-me) nick %s", + logging.Warn("irc.MODE(): recieved MODE %s for (non-me) nick %s", line.Args[1], line.Args[0]) return } nk.ParseModes(line.Args[1]) } else { - conn.l.Warn("irc.MODE(): not sure what to do with MODE %s", + logging.Warn("irc.MODE(): not sure what to do with MODE %s", strings.Join(line.Args, " ")) } } @@ -121,7 +122,7 @@ func (conn *Conn) h_TOPIC(line *Line) { if ch := conn.ST.GetChannel(line.Args[0]); ch != nil { ch.Topic = line.Args[1] } else { - conn.l.Warn("irc.TOPIC(): topic change on unknown channel %s", + logging.Warn("irc.TOPIC(): topic change on unknown channel %s", line.Args[0]) } } @@ -133,7 +134,7 @@ func (conn *Conn) h_311(line *Line) { nk.Host = line.Args[3] nk.Name = line.Args[5] } else { - conn.l.Warn("irc.311(): received WHOIS info for unknown nick %s", + logging.Warn("irc.311(): received WHOIS info for unknown nick %s", line.Args[1]) } } @@ -143,7 +144,7 @@ func (conn *Conn) h_324(line *Line) { if ch := conn.ST.GetChannel(line.Args[1]); ch != nil { ch.ParseModes(line.Args[2], line.Args[3:]...) } else { - conn.l.Warn("irc.324(): received MODE settings for unknown channel %s", + logging.Warn("irc.324(): received MODE settings for unknown channel %s", line.Args[1]) } } @@ -153,7 +154,7 @@ func (conn *Conn) h_332(line *Line) { if ch := conn.ST.GetChannel(line.Args[1]); ch != nil { ch.Topic = line.Args[2] } else { - conn.l.Warn("irc.332(): received TOPIC value for unknown channel %s", + logging.Warn("irc.332(): received TOPIC value for unknown channel %s", line.Args[1]) } } @@ -175,7 +176,7 @@ func (conn *Conn) h_352(line *Line) { nk.Modes.Invisible = true } } else { - conn.l.Warn("irc.352(): received WHO reply for unknown nick %s", + logging.Warn("irc.352(): received WHO reply for unknown nick %s", line.Args[5]) } } @@ -219,7 +220,7 @@ func (conn *Conn) h_353(line *Line) { } } } else { - conn.l.Warn("irc.353(): received NAMES list for unknown channel %s", + logging.Warn("irc.353(): received NAMES list for unknown channel %s", line.Args[2]) } } @@ -229,7 +230,7 @@ func (conn *Conn) h_671(line *Line) { if nk := conn.ST.GetNick(line.Args[1]); nk != nil { nk.Modes.SSL = true } else { - conn.l.Warn("irc.671(): received WHOIS SSL info for unknown nick %s", + logging.Warn("irc.671(): received WHOIS SSL info for unknown nick %s", line.Args[1]) } }