From 580be67e4c5f809ef8f148b40b17c2ea525be1fc Mon Sep 17 00:00:00 2001 From: Kaleb Elwert Date: Tue, 10 Apr 2018 13:38:37 -0700 Subject: [PATCH 1/3] Move client code out into separate filter functions gometalinter (specifically the gocyclo check) doesn't seem to handle functions inside a dict very well. Splitting them out makes it easier to deal with and makes them easier to test. --- client.go | 90 -------------------------------- client_handlers.go | 124 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 90 deletions(-) create mode 100644 client_handlers.go diff --git a/client.go b/client.go index c5d504d..999ebff 100644 --- a/client.go +++ b/client.go @@ -5,100 +5,10 @@ import ( "errors" "fmt" "io" - "strings" "sync" "time" ) -// clientFilters are pre-processing which happens for certain message -// types. These were moved from below to keep the complexity of each -// component down. -var clientFilters = map[string]func(*Client, *Message){ - "001": func(c *Client, m *Message) { - c.currentNick = m.Params[0] - c.connected = true - }, - "433": func(c *Client, m *Message) { - // We only want to try and handle nick collisions during the initial - // handshake. - if c.connected { - return - } - c.currentNick = c.currentNick + "_" - c.Writef("NICK :%s", c.currentNick) - }, - "437": func(c *Client, m *Message) { - // We only want to try and handle nick collisions during the initial - // handshake. - if c.connected { - return - } - c.currentNick = c.currentNick + "_" - c.Writef("NICK :%s", c.currentNick) - }, - "PING": func(c *Client, m *Message) { - reply := m.Copy() - reply.Command = "PONG" - c.WriteMessage(reply) - }, - "PONG": func(c *Client, m *Message) { - if c.incomingPongChan != nil { - select { - case c.incomingPongChan <- m.Trailing(): - default: - } - } - }, - "NICK": func(c *Client, m *Message) { - if m.Prefix.Name == c.currentNick && len(m.Params) > 0 { - c.currentNick = m.Params[0] - } - }, - "CAP": func(c *Client, m *Message) { - if c.remainingCapResponses <= 0 || len(m.Params) <= 2 { - return - } - - switch m.Params[1] { - case "LS": - for _, key := range strings.Split(m.Trailing(), " ") { - cap := c.caps[key] - cap.Available = true - c.caps[key] = cap - } - c.remainingCapResponses-- - case "ACK": - for _, key := range strings.Split(m.Trailing(), " ") { - cap := c.caps[key] - cap.Enabled = true - c.caps[key] = cap - } - c.remainingCapResponses-- - case "NAK": - // If we got a NAK and this REQ was required, we need to bail - // with an error. - for _, key := range strings.Split(m.Trailing(), " ") { - if c.caps[key].Required { - c.sendError(fmt.Errorf("CAP %s requested but was rejected", key)) - return - } - } - c.remainingCapResponses-- - } - - if c.remainingCapResponses <= 0 { - for key, cap := range c.caps { - if cap.Required && !cap.Enabled { - c.sendError(fmt.Errorf("CAP %s requested but not accepted", key)) - return - } - } - - c.Write("CAP END") - } - }, -} - // ClientConfig is a structure used to configure a Client. type ClientConfig struct { // General connection information. diff --git a/client_handlers.go b/client_handlers.go new file mode 100644 index 0000000..cacd6e3 --- /dev/null +++ b/client_handlers.go @@ -0,0 +1,124 @@ +package irc + +import ( + "fmt" + "strings" +) + +type clientFilter func(*Client, *Message) + +// clientFilters are pre-processing which happens for certain message +// types. These were moved from below to keep the complexity of each +// component down. +var clientFilters = map[string]clientFilter{ + "001": handle001, + "433": handle433, + "437": handle437, + "PING": handlePing, + "PONG": handlePong, + "NICK": handleNick, + "CAP": handleCap, +} + +func handle001(c *Client, m *Message) { + c.currentNick = m.Params[0] + c.connected = true +} + +func handle433(c *Client, m *Message) { + // We only want to try and handle nick collisions during the initial + // handshake. + if c.connected { + return + } + c.currentNick = c.currentNick + "_" + c.Writef("NICK :%s", c.currentNick) +} + +func handle437(c *Client, m *Message) { + // We only want to try and handle nick collisions during the initial + // handshake. + if c.connected { + return + } + c.currentNick = c.currentNick + "_" + c.Writef("NICK :%s", c.currentNick) +} + +func handlePing(c *Client, m *Message) { + reply := m.Copy() + reply.Command = "PONG" + c.WriteMessage(reply) +} + +func handlePong(c *Client, m *Message) { + if c.incomingPongChan != nil { + select { + case c.incomingPongChan <- m.Trailing(): + default: + } + } +} + +func handleNick(c *Client, m *Message) { + if m.Prefix.Name == c.currentNick && len(m.Params) > 0 { + c.currentNick = m.Params[0] + } +} + +var capFilters = map[string]clientFilter{ + "LS": handleCapLs, + "ACK": handleCapAck, + "NAK": handleCapNak, +} + +func handleCap(c *Client, m *Message) { + if c.remainingCapResponses <= 0 || len(m.Params) <= 2 { + return + } + + if filter, ok := capFilters[m.Params[1]]; ok { + filter(c, m) + } + + if c.remainingCapResponses <= 0 { + for key, cap := range c.caps { + if cap.Required && !cap.Enabled { + c.sendError(fmt.Errorf("CAP %s requested but not accepted", key)) + return + } + } + + c.Write("CAP END") + } +} + +func handleCapLs(c *Client, m *Message) { + for _, key := range strings.Split(m.Trailing(), " ") { + cap := c.caps[key] + cap.Available = true + c.caps[key] = cap + } + c.remainingCapResponses-- +} + +func handleCapAck(c *Client, m *Message) { + for _, key := range strings.Split(m.Trailing(), " ") { + cap := c.caps[key] + cap.Enabled = true + c.caps[key] = cap + } + c.remainingCapResponses-- +} + +func handleCapNak(c *Client, m *Message) { + // If we got a NAK and this REQ was required, we need to bail + // with an error. + for _, key := range strings.Split(m.Trailing(), " ") { + if c.caps[key].Required { + c.sendError(fmt.Errorf("CAP %s requested but was rejected", key)) + return + } + } + c.remainingCapResponses-- +} From 01700c2b2312e42b6d6d28424589a597901623ff Mon Sep 17 00:00:00 2001 From: Kaleb Elwert Date: Tue, 10 Apr 2018 14:07:31 -0700 Subject: [PATCH 2/3] Update TestPingLoop to properly test handlePong --- client_handlers.go | 3 +++ client_test.go | 14 ++++---------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/client_handlers.go b/client_handlers.go index cacd6e3..6456b2e 100644 --- a/client_handlers.go +++ b/client_handlers.go @@ -56,6 +56,9 @@ func handlePong(c *Client, m *Message) { select { case c.incomingPongChan <- m.Trailing(): default: + // Note that this return isn't really needed, but it helps some code + // coverage tools actually see this line. + return } } } diff --git a/client_test.go b/client_test.go index 68e6d7d..523ffd4 100644 --- a/client_test.go +++ b/client_test.go @@ -388,20 +388,14 @@ func TestPingLoop(t *testing.T) { // This one is just for coverage, so we know we're hitting the // branch that drops extra pings. - runClientTest(t, config, io.EOF, nil, []TestAction{ + runClientTest(t, config, io.EOF, func(c *Client) { + c.incomingPongChan = make(chan string) + handlePong(c, MustParseMessage("PONG :hello 1")) + }, []TestAction{ ExpectLine("PASS :test_pass\r\n"), ExpectLine("NICK :test_nick\r\n"), ExpectLine("USER test_user 0.0.0.0 0.0.0.0 :test_name\r\n"), SendLine("001 :hello_world\r\n"), - - // It's a buffered channel of 5, so we want to send at least 6 of them - SendLine("PONG :hello 1\r\n"), - SendLine("PONG :hello 2\r\n"), - SendLine("PONG :hello 3\r\n"), - SendLine("PONG :hello 4\r\n"), - SendLine("PONG :hello 5\r\n"), - SendLine("PONG :hello 6\r\n"), - SendLine("PONG :hello 7\r\n"), }) // Successful ping with write error From 636832b54c4f6aa8f65059490ba31f0c00f493e7 Mon Sep 17 00:00:00 2001 From: Kaleb Elwert Date: Tue, 10 Apr 2018 14:16:12 -0700 Subject: [PATCH 3/3] Update external testcases repo --- parser_test.go | 22 +++++++++++++--------- testcases | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/parser_test.go b/parser_test.go index e25ae71..7b45cd6 100644 --- a/parser_test.go +++ b/parser_test.go @@ -113,6 +113,7 @@ func TestMessageCopy(t *testing.T) { type MsgSplitTests struct { Tests []struct { + Desc string Input string Atoms struct { Source *string @@ -135,15 +136,15 @@ func TestMsgSplit(t *testing.T) { for _, test := range splitTests.Tests { msg, err := ParseMessage(test.Input) - assert.NoError(t, err, "Failed to parse: %s (%s)", test.Input, err) + assert.NoError(t, err, "%s: Failed to parse: %s (%s)", test.Desc, test.Input, err) assert.Equal(t, strings.ToUpper(test.Atoms.Verb), msg.Command, - "Wrong command for input: %s", test.Input, + "%s: Wrong command for input: %s", test.Desc, test.Input, ) assert.Equal(t, test.Atoms.Params, msg.Params, - "Wrong params for input: %s", test.Input, + "%s: Wrong params for input: %s", test.Desc, test.Input, ) if test.Atoms.Source != nil { @@ -152,16 +153,17 @@ func TestMsgSplit(t *testing.T) { assert.Equal(t, len(test.Atoms.Tags), len(msg.Tags), - "Wrong number of tags", + "%s: Wrong number of tags", + test.Desc, ) for k, v := range test.Atoms.Tags { tag, ok := msg.GetTag(k) assert.True(t, ok, "Missing tag") if v == nil { - assert.EqualValues(t, "", tag, "Tag differs") + assert.EqualValues(t, "", tag, "%s: Tag %q differs: %s != \"\"", test.Desc, k, tag) } else { - assert.EqualValues(t, v, tag, "Tag differs") + assert.EqualValues(t, v, tag, "%s: Tag %q differs: %s != %s", test.Desc, k, v, tag) } } } @@ -169,6 +171,7 @@ func TestMsgSplit(t *testing.T) { type MsgJoinTests struct { Tests []struct { + Desc string Atoms struct { Source string Verb string @@ -211,6 +214,7 @@ func TestMsgJoin(t *testing.T) { type UserhostSplitTests struct { Tests []struct { + Desc string Source string Atoms struct { Nick string @@ -235,15 +239,15 @@ func TestUserhostSplit(t *testing.T) { assert.Equal(t, test.Atoms.Nick, prefix.Name, - "Name did not match for input: %q", test.Source, + "%s: Name did not match for input: %q", test.Desc, test.Source, ) assert.Equal(t, test.Atoms.User, prefix.User, - "User did not match for input: %q", test.Source, + "%s: User did not match for input: %q", test.Desc, test.Source, ) assert.Equal(t, test.Atoms.Host, prefix.Host, - "Host did not match for input: %q", test.Source, + "%s: Host did not match for input: %q", test.Desc, test.Source, ) } } diff --git a/testcases b/testcases index cb0cf57..350a8ce 160000 --- a/testcases +++ b/testcases @@ -1 +1 @@ -Subproject commit cb0cf5737a8ba01feff4e3423d87fb356258f361 +Subproject commit 350a8cec9ba3993e87c707c6e3119033b03faa85