954902 - Add unit tests for building IRC messages

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1469 at 2012-05-26 13:16:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Tests v1

*** Original post on bio 1469 as attmnt 1530 at 2012-05-26 13:16:00 UTC ***

This adds a bunch of tests that parse string messages to objects and then rebuild the string messages. The test cases are just the messages from RFC 2812 + a few specially crafted ones. If there are other messages that have given us issues in the past, please point me to them!

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 1469 at 2012-05-28 16:09:16 UTC ***

Comment on attachment 8353284 [details] [diff] [review] (bio-attmnt 1530)
Tests v1

>+function run_test() {
>+  print(irc.ircMessage);

You wanted to remove this line, didn't you?

>+
>+  for each (let expectedStringMessage in testData) {
>+    let message = irc.ircMessage(expectedStringMessage);
>+
>+    let stringMessage =
>+      irc.ircAccount.prototype.buildMessage(message.command, message.params);

So if I understand correctly, we parse an IRC message, and then pretty print it to compare them. Is it possible to have some cases where a bug in the parser could be hidden to the test by the same (of the opposite) bug in the pretty printer?

I'm wondering if we also need to somehow test going from a string as provided provided by the user to an ircMessage and then back to the string of the message.

It seems that we are only prepending the "PRIVMSG <nick>" string, so I guess the chances of such a bug is unlikely.

>+
>+    do_check_eq(stringMessage, expectedStringMessage);
>+  }
>+
>+  //run_next_test();

And remove this one too.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 1469 at 2012-05-29 21:49:32 UTC ***

(In reply to comment #1)
> Comment on attachment 8353284 [details] [diff] [review] (bio-attmnt 1530) [details]
> Tests v1
> 
> >+function run_test() {
> >+  print(irc.ircMessage);
> 
> You wanted to remove this line, didn't you?
> 
> >+
> >+  for each (let expectedStringMessage in testData) {
> >+    let message = irc.ircMessage(expectedStringMessage);
> >+
> >+    let stringMessage =
> >+      irc.ircAccount.prototype.buildMessage(message.command, message.params);
> 
> So if I understand correctly, we parse an IRC message, and then pretty print it
> to compare them. Is it possible to have some cases where a bug in the parser
> could be hidden to the test by the same (of the opposite) bug in the pretty
> printer?
We parse the IRC message from a string (as if we received it), then rebuild the message (as if we were sending it) and then compare them. Is that what you mean by "pretty print"? It is possible that a bug in one could be hidden by the other, yes.

> I'm wondering if we also need to somehow test going from a string as provided
> provided by the user to an ircMessage and then back to the string of the
> message.
Do you mean specifically testing the PRIVMSG functionality then?

> It seems that we are only prepending the "PRIVMSG <nick>" string, so I guess
> the chances of such a bug is unlikely.
Yes, that's pretty much all it does. :)

I've removed the two garbage lines and can put up a new patch if there's no further questions.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v1.1

*** Original post on bio 1469 as attmnt 1535 at 2012-05-29 22:04:00 UTC ***

With the debug code removed.

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8353284 [details] [diff] [review]
Tests v1

*** Original change on bio 1469 attmnt 1530 at 2012-05-29 22:04:12 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8353289 [details] [diff] [review]
Patch v1.1

*** Original change on bio 1469 attmnt 1535 at 2012-05-29 22:21:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[Patrick Cloke [:clokep]]

*** Original post on bio 1469 at 2012-05-29 22:37:39 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/0994b814b441