Fix broken html report when benchmark names contain '\''

[jhrcek]

Proposed fix for #202

[RyanGlScott]

While this works, it might actually be a bit overkill. Why not just change the single quotes to double quotes, like so?

diff --git a/templates/default.tpl b/templates/default.tpl
index 5366899..95bdee1 100644
--- a/templates/default.tpl
+++ b/templates/default.tpl
@@ -287,7 +287,7 @@ $(function () {
   reports.map(mangulate);
 
   var benches = [{{#report}}"{{name}}",{{/report}}];
-  var ylabels = [{{#report}}[-{{number}},'<a href="#b{{number}}">{{name}}</a>'],{{/report}}];
+  var ylabels = [{{#report}}[-{{number}},"<a href=\"#b{{number}}\">{{name}}</a>"],{{/report}}]
   var means = $.scaleTimes([{{#report}}{{anMean.estPoint}},{{/report}}]);
   var stddevs = [{{#report}}{{anStdDev.estPoint}},{{/report}}];
   stddevs = $.scaleBy(means[1], stddevs);

This bypasses the need to escape single quotes within name entirely (since single quotes don't need to be escaped when they're between double quotes like this).

[jhrcek]

Good point. But wouldn't that create the same problem when using benchmark names containing double quotes? Anyway I'll try and also will try to write some tests.

I'll work on this next weekend. I've you could suggest off the top of your head how this could be tested (at the right level of abstraction - how to check the generated stuff passes as valid javascript no matter what appears in benchmark's name?) It would be great. Otherwise I'll take this as an exercise for myself and try to figure something out :-)

[RyanGlScott]

Good point. But wouldn't that create the same problem when using benchmark names containing double quotes?

microstache does escape double quotes, so no, that wouldn't be a problem.

I've you could suggest off the top of your head how this could be tested (at the right level of abstraction - how to check the generated stuff passes as valid javascript no matter what appears in benchmark's name?)

That's a good question, but I'm not sure of the answer myself. We already have one test (Sanity.hs) which ensures that an HTML report is generated. However, it doesn't do any additional validation of the generated report. I imagine we would need some HTML + JavaScript validation library to actually ensure that the contents of the report are well formed, but I'm not very familiar with that part of the Haskell ecosystem.

[jhrcek]

Hello @RyanGlScott
I changed the fix according to your suggestion and then investigate possibilities of writing automated test for it. I conclude that it would be too heavyweight to automate some kind of "interpret if the report is valid HTML + JS" just to verify this trivial fix.

Instead I just locally modified the Sanity.hs to generate reports for the first 10 000 characters

first10kChars = take 10000 [minBound::Char ..]

and then manually verified (by opening the report in the browser) which of these chars break the charts in the report, comparing the previous and fixed implementation.

Results:
The generated report is valid with all the tested chars, except for the following four characters :

Dec	Character	Explanation
10	\n	New line character breaks the template, as javascript string can't contain verbatim newline char
13	\r	Carriage return - same as above
39	'	Apostrophe - broke the previous implementation, but works fine with this PR
92	\	backslash - breaks the report only when at the end of the benchmark name, because it escapes the " that is supposed to end the string, but after escaping no longer does

Even though it fixes just one char (and breaks no others) I'd suggest we merge this, because I feel it's relatively more likely that people include apostrophe in the benchmark name,

[RyanGlScott]

Thanks for the thorough testing! I agree that it's probably best to just merge this as-is.