Closed Bug 954929 Opened 7 years ago Closed 7 years ago

test_ctcpFormatToText and test_mIRCColor aren't executed

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: clokep)

References

()

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 1496 at 2012-06-08 15:59:00 UTC ***

I'm not sure if it's intentional or not (if it's intentional I think it would be a good idea to at least have a comment saying why) but the tests test_ctcpFormatToText and test_mIRCColor aren't executed.
*** Original post on bio 1496 at 2012-06-10 19:33:29 UTC ***

(In reply to comment #0)
> I'm not sure if it's intentional or not (if it's intentional I think it would
> be a good idea to at least have a comment saying why) but the tests
> test_ctcpFormatToText and test_mIRCColor aren't executed.

No idea why these aren't executed. Turns out these two are the same test though.

The fun part is that the test fails. :) Working on this now.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
OS: Other → All
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1496 as attmnt 1586 at 2012-06-10 21:02:00 UTC ***

So this removes the duplicated test and enables the other test, plus fixes the result of what that test should return. (Pretty much just all formatting stripped out.)

This also makes the ctcpFormatToText method much more thorough, so it can handle mIRC color codes as well. (This was not previously handled.)
Attachment #8353343 - Flags: review?(florian)
*** Original post on bio 1496 at 2012-06-10 22:14:59 UTC ***

Comment on attachment 8353343 [details] [diff] [review] (bio-attmnt 1586)
Patch v1


>   let output = input.map(ctcpFormatToText);
>   for (let i = 0; i < output.length; i++)
>-    do_check_eq(expectedOutput, output[i]);
>+    do_check_eq(expectedOutput[i], output[i]);

If ctcpFormatToText throws for any of the input values, we will never know which one from the log of the test execution, because all executions are done before testing their results. I think the map call here should disappear. Is there an reason for not doing this?
do_check_eq(expectedOutput[i], ctcpFormatToText(input[i]));


Note: I haven't read the rest of the patch, so you may not want to attach another version now just for that :).
Comment on attachment 8353343 [details] [diff] [review]
Patch v1

*** Original change on bio 1496 attmnt 1586 at 2012-06-23 22:58:54 UTC ***

>+/*
>+ * The supported formatting control characters, as described as deprecated in
>+ * http://www.invlogic.com/irc/ctcp.html#3.11
>+ * If a string is given, it will replace the control character; if null is
>+ * given, the current HTML tags stack will be closed; if a function is given,

"tags stack" or "tag stack"?

>+ * it expects three parameters:
>+ *  aStack  The sorted list of open HTML tags.
>+ *  aInput  The current input string.
>+ *  aOutput The current output string.

It's not really clear to me why that function needs the output as a parameter.

>+ * There are three output variables:
>+ *  stack   The new sorted list of open HTML tags.

Did you mean "ordered" instead of "sorted"?

>+ *  output  The new text output.

Shouldn't the function return a value to append to the already processed output, instead of returning a new output string?

>+ *  length  The number of characters (from the start of the string) that the
>+ *          function handled.
"length" 'feels' wrong here as a name, but I'm not sure 'offset' would really be better.
>+ */

Anyway, thanks or adding documentation here! :)

> const CTCP_TAGS = {"\x02": "b", // \002, ^B, Bold
>                    "\x16": "i", // \026, ^V, Reverse or Inverse (Italics)
>                    "\x1F": "u", // \037, ^_, Underline
>@@ -28,12 +41,37 @@ const CTCP_TAGS = {"\x02": "b", // \002,
>                    "\x0F": null}; // \017, ^O, Clear all formatting
> 
> // Generate an expression that will search for any of the control characters.
>-const CTCP_TAGS_STRING = "[" + Object.keys(CTCP_TAGS).join("") + "]";
>-const CTCP_TAGS_EXP = new RegExp(CTCP_TAGS_STRING);
>-const CTCP_TAGS_EXP_GLOBAL = new RegExp(CTCP_TAGS_STRING, "g");
>+const CTCP_TAGS_EXP = new RegExp("[" + Object.keys(CTCP_TAGS).join("") + "]");
> 
> // Remove all CTCP formatting characters.
>-function ctcpFormatToText(aString) aString.replace(CTCP_TAGS_EXP_GLOBAL, "")
>+function ctcpFormatToText(aString) {
>+  let next,
>+      input = aString,
>+      output = "",
>+      length;
>+
>+  // Used when calling functions.
>+  let stack = [];
>+  let temp1, temp2;

There were really no possible better names (if we need these variables at all)?

>+  while ((next = CTCP_TAGS_EXP.exec(input))) {
>+    if (next.index > 0)
>+      output += input.substr(0, next.index);
>+    // We assume one character will be stripped (this is the null case or the
>+    // single character case).
>+    length = 1;
>+    let tag = CTCP_TAGS[input[next.index]];
>+    if (typeof tag == "function")
>+      [temp1, temp2, length] = tag(stack, input.substr(next.index), output);

Isn't this equivalent to:
 [, , length] = tag([], input.substr(next.index), "");


>+
>+    // Avoid infinite loops, if.
>+    length = (length <= 0) ? 1 : length;

What about:
length = Math.max(1, length);

>+    // Skip to after the last match.
>+    input = input.substr(next.index + length);
>+  }
>+  // Return unmatched bits.

This comment isn't really what you are doing ;).
what about "append the unmatched bits before returning the output"?

>+  return output + input;
>+}
Attachment #8353343 - Flags: review?(florian) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1496 as attmnt 1682 at 2012-06-24 02:57:00 UTC ***

(In reply to comment #4)
> Comment on attachment 8353343 [details] [diff] [review] (bio-attmnt 1586) [details]
> >+ * it expects three parameters:
> >+ *  aStack  The sorted list of open HTML tags.
> >+ *  aInput  The current input string.
> >+ *  aOutput The current output string.
> 
> It's not really clear to me why that function needs the output as a parameter.
It really doesn't, I changed it to append. (I had thought of doing this previously, but wanted to get the tests working first!)

> >+ * There are three output variables:
> >+ *  stack   The new sorted list of open HTML tags.
> 
> Did you mean "ordered" instead of "sorted"?
Yes. :)

> >+ *  length  The number of characters (from the start of the string) that the
> >+ *          function handled.
> "length" 'feels' wrong here as a name, but I'm not sure 'offset' would really
> be better.
> >+ */
I agree, but I couldn't think of a better name.

> Anyway, thanks or adding documentation here! :)
:) Documentation is good.

> >+  while ((next = CTCP_TAGS_EXP.exec(input))) {
> >+    if (next.index > 0)
> >+      output += input.substr(0, next.index);
> >+    // We assume one character will be stripped (this is the null case or the
> >+    // single character case).
> >+    length = 1;
> >+    let tag = CTCP_TAGS[input[next.index]];
> >+    if (typeof tag == "function")
> >+      [temp1, temp2, length] = tag(stack, input.substr(next.index), output);
> 
> Isn't this equivalent to:
>  [, , length] = tag([], input.substr(next.index), "");
So I had no idea you could give empty output parameters in JavaScript. Yes, that's what I wanted.

> >+
> >+    // Avoid infinite loops, if.
> >+    length = (length <= 0) ? 1 : length;
> 
> What about:
> length = Math.max(1, length);
Yes, that's equivalent. I can change it to that if you'd like (but I was matching the previous code). I'll change both.

> >+    // Skip to after the last match.
> >+    input = input.substr(next.index + length);
> >+  }
> >+  // Return unmatched bits.
> 
> This comment isn't really what you are doing ;).
> what about "append the unmatched bits before returning the output"?
Attachment #8353439 - Flags: review?
Comment on attachment 8353343 [details] [diff] [review]
Patch v1

*** Original change on bio 1496 attmnt 1586 at 2012-06-24 02:57:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353343 - Attachment is obsolete: true
Comment on attachment 8353439 [details] [diff] [review]
Patch v2

*** Original change on bio 1496 attmnt 1682 at 2012-06-24 02:59:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353439 - Flags: review? → review?(florian)
Comment on attachment 8353439 [details] [diff] [review]
Patch v2

*** Original change on bio 1496 attmnt 1682 at 2012-06-24 10:31:46 UTC ***

>+ * The supported formatting control characters, as described in
>+ * http://www.invlogic.com/irc/ctcp.html#3.11
>+ * If a string is given, it will replace the control character; if null is
>+ * given, the current HTML tag stack will be closed; if a function is given,
>+ * it expects three parameters:

Three parameters, really?

>+ * There are three output variables:
>+ *  stack   The new ordered list of open HTML tags.
>+ *  output  The new text output to append.
>+ *  length  The number of characters (from the start of the string) that the
>+ *          function handled.

The names seem a bit irrelevant here as we are returning an array rather than an object containing variables.

Maybe just say "There are three output values returned in an array:" and drop the names?

>+    // Append the unmatched bits before returning the output.
>+    input = input.substr(next.index + length);
>+  }
>+  // Return unmatched bits.
>+  return output + input;

The "Append the unmatched bits before returning the output." comment that I proposed during my previous review was meant to replace "Return unmatched bits.". It doesn't seem to mean anything above the line of code where you put it :-S.
Attachment #8353439 - Flags: review?(florian) → review-
Attached patch Patch v3Splinter Review
*** Original post on bio 1496 as attmnt 1685 at 2012-06-24 14:12:00 UTC ***

I went over the entire file again and added a couple of comments, removed some duplication (with the openStack function).

Also meets your review comments. :)
Attachment #8353442 - Flags: review?(florian)
Comment on attachment 8353439 [details] [diff] [review]
Patch v2

*** Original change on bio 1496 attmnt 1682 at 2012-06-24 14:12:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353439 - Attachment is obsolete: true
Comment on attachment 8353442 [details] [diff] [review]
Patch v3

*** Original change on bio 1496 attmnt 1685 at 2012-06-26 09:02:37 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353442 - Flags: review?(florian) → review+
*** Original post on bio 1496 at 2012-06-26 23:49:50 UTC ***

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