Bug 86931

Summary: LayoutTests: fast/js/resources/js-test-pre.js - shouldBeEqualToString fails on "
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: Tools / TestsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dglazkov, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch for landing none

Description Joshua Bell 2012-05-18 18:02:55 PDT
function shouldBeEqualToString(a, b)
{
  var unevaledString = '"' + b.replace(/\\/g, "\\\\").replace(/"/g, "\"").replace(/\n/g, "\\n").replace(/\r/g, "\\r") + '"';
  shouldBe(a, unevaledString);
}

That replace(/"/g, "\"") should be replace(/"/g, "\\"")

Or just replace that whole line with:

  var unevaledString = JSON.stringify(b);
Comment 1 Joshua Bell 2012-05-21 10:52:37 PDT
Created attachment 143058 [details]
Patch
Comment 2 Joshua Bell 2012-05-21 10:56:33 PDT
I'm expecting a handful of tests to 'asplode as a result of this change - anything that was relying on the broken escaping behavior of shouldBeEqualToString. Let's see what the bots say.

Alec, can you sanity check the IDB test change?
Comment 3 Ojan Vafai 2012-05-21 11:14:39 PDT
Comment on attachment 143058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143058&action=review

> LayoutTests/fast/js/resources/js-test-pre.js:261
> +  var unevaledString = JSON.stringify(b);

On the one hand, this is simpler, on the other it means that if you're explicitly testing that you get a string and not and object that stringifies to the same thing, then this won't work. 

Can we add a typeof b == 'string' check?
Comment 4 Alec Flett 2012-05-21 11:26:05 PDT
Glad to see this go in, I struggled to get this right and it drove me nuts - LGTM (agree on the typeof comment from ojan as well)
Comment 5 Joshua Bell 2012-05-21 11:40:22 PDT
(In reply to comment #3)
> On the one hand, this is simpler, on the other it means that if you're explicitly testing that you get a string and not and object that stringifies to the same thing, then this won't work. 
> 
> Can we add a typeof b == 'string' check?

Agreed and will do. Shouldn't break anything either, as b.replace() was previously called without a String() coercion.
Comment 6 WebKit Review Bot 2012-05-21 11:41:49 PDT
Comment on attachment 143058 [details]
Patch

Attachment 143058 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12749018

New failing tests:
storage/domstorage/complex-values.html
fast/dom/HTMLElement/class-list.html
fast/dom/HTMLElement/class-list-quirks.html
fast/dom/HTMLOutputElement/dom-settable-token-list.html
Comment 7 WebKit Review Bot 2012-05-21 11:41:52 PDT
Created attachment 143068 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Joshua Bell 2012-05-21 11:47:37 PDT
As expected...

(In reply to comment #6)
> New failing tests:
> storage/domstorage/complex-values.html

PASS->PASS, but output differs: contains an embedded NUL, was printed as \0 now printed as \u0000

> fast/dom/HTMLElement/class-list.html

PASS->PASS, but output differs: contains an embedded TAB, was printed as tab character, now printed as \t

> fast/dom/HTMLElement/class-list-quirks.html

PASS->PASS, but output differs: contains an embedded TAB, was printed as tab character, now printed as \t

> fast/dom/HTMLOutputElement/dom-settable-token-list.html

PASS->PASS, but output differs: contains an embedded TAB, was printed as tab character, now printed as \t

Will rebase these tests.
Comment 9 Joshua Bell 2012-05-21 11:50:25 PDT
Created attachment 143069 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-05-21 12:19:29 PDT
Comment on attachment 143069 [details]
Patch for landing

Clearing flags on attachment: 143069

Committed r117806: <http://trac.webkit.org/changeset/117806>
Comment 11 WebKit Review Bot 2012-05-21 12:19:34 PDT
All reviewed patches have been landed.  Closing bug.