Bug 16806 - SunSpider/tests/string-base64.js does not compute a valid base64 encoded string
Summary: SunSpider/tests/string-base64.js does not compute a valid base64 encoded string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-09 14:06 PST by Eric Seidel (no email)
Modified: 2009-12-14 14:26 PST (History)
5 users (show)

See Also:


Attachments
fix correctness and add correctness check (2.88 KB, patch)
2008-01-09 14:16 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (3.99 KB, patch)
2009-12-13 17:34 PST, Maciej Stachowiak
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-09 14:06:39 PST
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.
Comment 1 Eric Seidel (no email) 2008-01-09 14:16:16 PST
Created attachment 18354 [details]
fix correctness and add correctness check
Comment 2 Darin Adler 2008-01-11 18:36:45 PST
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.
Comment 3 Darin Adler 2008-01-11 18:38:37 PST
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.
Comment 4 Eric Seidel (no email) 2008-01-13 11:08:39 PST
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.
Comment 5 Timothy Hatcher 2008-02-06 07:27:11 PST
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.
Comment 6 Eric Seidel (no email) 2008-02-06 10:55:17 PST
(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.
Comment 7 Eric Seidel (no email) 2008-05-30 08:35:12 PDT
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.
Comment 8 Darin Adler 2008-05-30 09:03:34 PDT
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".
Comment 9 Eric Seidel (no email) 2008-05-30 10:03:58 PDT
(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.
Comment 10 Maciej Stachowiak 2008-05-30 23:35:34 PDT
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.

Comment 11 Eric Seidel (no email) 2008-08-01 12:07:57 PDT
(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.
Comment 12 Maciej Stachowiak 2008-08-31 19:54:55 PDT
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.
Comment 13 Eric Seidel (no email) 2008-12-01 12:27:24 PST
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.
Comment 14 Gaurav Seth 2009-01-05 12:28:34 PST
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.
Comment 15 Maciej Stachowiak 2009-12-13 17:34:16 PST
Created attachment 44770 [details]
Patch
Comment 16 Gavin Barraclough 2009-12-13 17:39:24 PST
Comment on attachment 44770 [details]
Patch

Do you mean the sunspider-compare-results change to be there?
Comment 17 WebKit Review Bot 2009-12-13 17:39:29 PST
style-queue ran check-webkit-style on attachment 44770 [details] without any errors.
Comment 18 Maciej Stachowiak 2009-12-13 18:38:56 PST
Committed r52078: <http://trac.webkit.org/changeset/52078>
Comment 19 Eric Seidel (no email) 2009-12-14 10:21:16 PST
Is sunspider versioned now?  It's nice that this is finally fixed!
Comment 20 Gaurav Seth 2009-12-14 14:26:54 PST
How do we access the updated version of SunSpider tests that have this change?