Closed Bug 954902 Opened 8 years ago Closed 8 years ago

Add unit tests for building IRC messages

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 1 obsolete file)

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

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Tests v1 (obsolete) — Splinter Review
*** 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!
Attachment #8353284 - Flags: review?
*** 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.
*** 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.
Attached patch Patch v1.1Splinter Review
*** Original post on bio 1469 as attmnt 1535 at 2012-05-29 22:04:00 UTC ***

With the debug code removed.
Attachment #8353289 - Flags: review?(florian)
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 ***
Attachment #8353284 - Attachment is obsolete: true
Attachment #8353284 - Flags: review?
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 ***
Attachment #8353289 - Flags: review?(florian) → review+
*** Original post on bio 1469 at 2012-05-29 22:37:39 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/0994b814b441
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.