Bug 16142 – Add --range option to run-sunspider

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Eric Seidel (no email)]

Add --range option to run-sunspider

I'd like to have information on exactly how the sunspider results changed, when.  to do that, I've added a --range option to sunspider which takes git/svn ranges and runs sunspider against all revisions in the provided range.

My current patch is incomplete, but I'll attach it anyway.  I'm not sure when I'll next have time to hack, hopefully tomorrow.

[Eric Seidel (no email)]

Created attachment 17527 [details]
partial fix

Next step is to add an HTML which actually knows how to use the output.  That will require sunspiderCompareResults() not *always* call printOutput, and instead return an object which can be told to print if needed.

[Eric Seidel (no email)]

Created attachment 17528 [details]
partial fix (which actually runs)

The previous patch was missing a semicolon.

[Eric Seidel (no email)]

Comment on attachment 17528 [details]
partial fix (which actually runs)

I think this is real enough for review.  I'll eventually come up with some sort of UI, but for now, just having a quick way to generate sunspider numbers per-build is incredibly useful.  Of course this half of the functionality (running something once for each build) really could be abstracted into a separate script.  But the later portion of building a custom UI to display all these will need to be in run-sunspider regardless.

[Adam Roben (:aroben)]

Comment on attachment 17528 [details]
partial fix (which actually runs)

 130 $resultsFile = $outputFilePath  if $outputFilePath;

Looks like you've got an extra space in there.

 128     return gitBranch()  if isGit();

Ditto.

 151         return split('\n', $revisionsString);
 152     } elsif (isSVN()) {

 154             return [$1];
 155         } elsif ($range =~ /^r?([0-9]+):?r?([0-9]+|HEAD)$/i) {

No need to say "else" after a return.

 156             return [$1..$2];

What does the range operator do if $1 or $2 is "HEAD"?

 61   --range           Get samples across the provided git range of builds

You shouldn't say "git" here.

 122 # Takes 0 or 1 args
 123 sub runUsingCurrentCheckout

You can still use a prototype:

sub runUsingCurrentCheckout(;$)

 179         last  unless ($result == 0);

Extra space here as well.

It seems silly to do two chdirs for every test run (plus an extra one at the start of the revision range). Can't we do the chdirs ahead of time and just do them once?

"--range" makes me think of a range of times or ranges of error -- maybe there's some more specific name we can give to this option? Maybe --revision-range?

r=me

[Eric Seidel (no email)]

Comment on attachment 17528 [details]
partial fix (which actually runs)

Going to clear the review flag given mjs's concerns about the format.  The next person who needs this functionality, and fix the patch.

[Ahmad Saleem]

@ap - Do we have sun spider as reference benchmark these days and do we need this? Thanks!

[Alexey Proskuryakov]

SunSpider is deeply obsolete, no need for this patch.