Bug 16806 – SunSpider/tests/string-base64.js does not compute a valid base64 encoded string

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

SunSpider/tests/string-base64.js does not compute a valid base64 encoded string

This patch to the test shows the error:

Index: string-base64.js
===================================================================
--- string-base64.js    (revision 29341)
+++ string-base64.js    (working copy)
@@ -126,7 +126,10 @@
     var base64;
 
     base64 = toBase64(str);
-    base64ToString(base64);
+    var roundtrip = base64ToString(base64);
+    if (str != roundtrip) {
+       throw "oops!";
+    }
 
     // Double the string
     str += str;

I'll attach a patch to fix the bug soon.  I expect this is already a well known issue.

[Eric Seidel (no email)]

Created attachment 18354 [details]
fix correctness and add correctness check

[Darin Adler]

Comment on attachment 18354 [details]
fix correctness and add correctness check

Looks fine to me. I don't know what Maciej's plan is for changes to SunSpider, though.

[Darin Adler]

What I mean is that I know he wants to fix issues like this one, but I do not know how he wants to stage the changes to SunSpider itself.

[Eric Seidel (no email)]

Comment on attachment 18354 [details]
fix correctness and add correctness check

Since Maciej is going to be the one making this change, I might as well make that clear in the review queue.

[Timothy Hatcher]

Maybe you should remove the string compare (str != roundtrip) before this lands. Do we want Sunspider to check correctness like this? Having the extra string compare in the test will make results differ more from the previous version.

[Eric Seidel (no email)]

(In reply to comment #5)
> Maybe you should remove the string compare (str != roundtrip) before this
> lands. Do we want Sunspider to check correctness like this? Having the extra
> string compare in the test will make results differ more from the previous
> version.
> 

Testing correctness is definitely my intention with this patch.  My recommendation would be that all sunspider tests should test themselves for correctness.

[Eric Seidel (no email)]

Comment on attachment 18354 [details]
fix correctness and add correctness check

Unless I hear a very loud NO from maciej, I'm going to find another reviewer and land this next week.  Silly to have SunSpider *still* be broken.

[Darin Adler]

I'm not sure that's helpful. We currently have SunSpider 0.9. I think it could be a mistake to turn our tree into "SunSpider 0.9 with some changes".

[Eric Seidel (no email)]

(In reply to comment #8)
> I'm not sure that's helpful. We currently have SunSpider 0.9. I think it could
> be a mistake to turn our tree into "SunSpider 0.9 with some changes".

I agree.  My previous comment wasn't very helpful.  Poor choice of venting other frustration and seeing that this bug is still awaiting review nearly 5 months later.  There are two reasons I can see for the delay.

1. mjs is swamped with other things.
2. mjs is intentionally avoiding landing changes to SunSpider.

1.  Is solved by finding another reviewer (it's a simple patch after all).
2.  Seems slightly broken by-design, since that's using trunk/ as a stable branch.

If 2 is the intention (and mjs and I can hash this out over IRC), then it seems that we could/should copy current SunSpider trunk into a branch, thus allowing further development.  An alternative would be to create a SunSpider-unstable branch, and develop there (but that's the opposite of how the rest of WebKit development works).

Either way, the status-quo is "don't touch sunspider" (or at least that's the vibe I've gotten from brief IRC conversations and having this patch sit for 3 months. :(

Again, mjs and I can (and will) hash this out on IRC.

[Maciej Stachowiak]

My apologies for not dealing with this patch yet. There are a couple of separate issues here:

1) trunk is not the frozen SunSpider 0.9 version; the copy on the web site is the frozen version
2) trunk SunSpider can change, but:
    2.a) we should think about a way to let people still easily run command-line against the old eversion (I don't think about that before) -- the web site copy does not have the command line version, so maybe there should be version subfolders in the SunSpider directory or something.
    2.b) I'd like to keep the tests balanced in time with each other in the reference browsers.

I can deal with 2.b (now firing up the PC laptop to test and adjust as needed) but I need to figure out something for 2.a.

As for the correctness check, I would like all the tests to have correctness checks (I have some code lying around to that effect), but ideally I would like the check to be outside the test itself (as well as, ideally having an input outside the test itself; the combination of the two things means a smart compiler can't just fold the whole test to a constant which would miss the point of the benchmark).

I'm sorry for having made myself the gatekeeper for SunSpider and not being responsive enough.

[Eric Seidel (no email)]

(In reply to comment #10)
Just checking in again... 2 months later.  I know you all are busy.  But this is one patch in the (very long) review queue which I can't review myself.

[Maciej Stachowiak]

I have not had a chance to do what I said in comment #10 yet. If you'd like to add support for multiple versions of the test content for SunSpider (with 0.9 still the default for now) then I'd be glad to review this patch immediately for 0.9.1. Otherwise, I (or anyone else) can review it once that infrastructure is in place.

Basically SunSpider has become important enough in the broader browser performance community that I don't think I can unilaterally judge what changes are good, so I think the next version of the benchmark should be staged for testing and put out for wider community review before it becomes official.

I'd appreciate your help on that.

[Eric Seidel (no email)]

Comment on attachment 18354 [details]
fix correctness and add correctness check

This bug doesn't need to sit in the review queue.  Maciej knows of the issue.  Sitting in the review queue for the next 11 months isn't going to solve the issue. :)  Maciej has noted reviewing this patch is blocked on other issues with sunspider distribution.

[Gaurav Seth]

This fix does not completely solve the problem of correctness of the base64 test (esp for IE). Reason being a non-standard compliance issue as detailed in the bug 22914.

[Maciej Stachowiak]

Created attachment 44770 [details]
Patch

[Gavin Barraclough]

Comment on attachment 44770 [details]
Patch

Do you mean the sunspider-compare-results change to be there?

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44770 [details] without any errors.

[Maciej Stachowiak]

Committed r52078: <http://trac.webkit.org/changeset/52078>

[Eric Seidel (no email)]

Is sunspider versioned now?  It's nice that this is finally fixed!

[Gaurav Seth]

How do we access the updated version of SunSpider tests that have this change?