Bug 58691

Summary: new-run-webkit-tests: sort test files "correctly"
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: Tools / TestsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 58625    
Attachments:
Description Flags
Patch
none
update w/ feedback from tony, restructure loop a bit
none
Patch
none
use a key= method instead of cmp=, rename to 'natural' sorting
none
fix path sorting as well
none
update w/ feedback from tony, remove extraneous extra sort none

Description Dirk Pranke 2011-04-15 13:54:17 PDT
ORWT sorts the test files such that foo-2.html is run before foo-10.html. NRWT should match this.
Comment 1 Dirk Pranke 2011-04-15 13:55:12 PDT
Created attachment 89848 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-15 13:55:43 PDT
I thought you had already posted a patch for this yesterday?
Comment 3 Eric Seidel (no email) 2011-04-15 13:58:38 PDT
Comment on attachment 89848 [details]
Patch

Seems OK to me.
Comment 4 Dirk Pranke 2011-04-15 13:59:34 PDT
(In reply to comment #2)
> I thought you had already posted a patch for this yesterday?

I did. Same patch, different bug.
Comment 5 Tony Chang 2011-04-17 14:01:12 PDT
Comment on attachment 89848 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:1240
> +            if int(x_part) < int(y_part):
> +                return -1
> +            elif int(x_part) > int(y_part):
> +                return 1

Nit: You could just do 'return cmp(int(x_part), int(y_part))'.  The same below.
Comment 6 Dirk Pranke 2011-04-17 14:34:56 PDT
Comment on attachment 89848 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:1240
>> +                return 1
> 
> Nit: You could just do 'return cmp(int(x_part), int(y_part))'.  The same below.

No, you can't, because you need to keep iterating if x_part == y_part.

You could do 'if cmp x, y: \ return cmp x,y', but I'm not sure if that's really any clearer.
Comment 7 Tony Chang 2011-04-17 15:24:29 PDT
Comment on attachment 89848 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:1240
>>> +                return 1
>> 
>> Nit: You could just do 'return cmp(int(x_part), int(y_part))'.  The same below.
> 
> No, you can't, because you need to keep iterating if x_part == y_part.
> 
> You could do 'if cmp x, y: \ return cmp x,y', but I'm not sure if that's really any clearer.

Ah, I see, I missed that you wanted to continue in that case.  You could do:
  if re.match('^\d', x_part) and re.match('^\d', y_part) and int(x_part) != int(y_part):
    return cmp(int(x_part), int(y_part))
Comment 8 Dirk Pranke 2011-04-17 16:48:55 PDT
(In reply to comment #7)
> (From update of attachment 89848 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89848&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_runner.py:1240
> >>> +                return 1
> >> 
> >> Nit: You could just do 'return cmp(int(x_part), int(y_part))'.  The same below.
> > 
> > No, you can't, because you need to keep iterating if x_part == y_part.
> > 
> > You could do 'if cmp x, y: \ return cmp x,y', but I'm not sure if that's really any clearer.
> 
> Ah, I see, I missed that you wanted to continue in that case.  You could do:
>   if re.match('^\d', x_part) and re.match('^\d', y_part) and int(x_part) != int(y_part):
>     return cmp(int(x_part), int(y_part))

OK.
Comment 9 Dirk Pranke 2011-04-17 16:54:13 PDT
Created attachment 89973 [details]
update w/ feedback from tony, restructure loop a bit
Comment 10 Dirk Pranke 2011-05-30 16:59:59 PDT
Created attachment 95383 [details]
Patch
Comment 11 Ryosuke Niwa 2011-06-06 23:54:59 PDT
Is this patch up for a review or not?
Comment 12 Dirk Pranke 2011-06-07 08:55:02 PDT
(In reply to comment #11)
> Is this patch up for a review or not?

No need. Tony R+'ed the earlier version, and the last version just had his feedback. It hasn't landed because it's waiting on some other work before it can go in without causing regressions on the chromium ports.
Comment 13 Dirk Pranke 2011-06-14 18:40:27 PDT
Created attachment 97215 [details]
use a key= method instead of cmp=, rename to 'natural' sorting
Comment 14 Dirk Pranke 2011-06-14 20:11:24 PDT
Created attachment 97223 [details]
fix path sorting as well
Comment 15 Dirk Pranke 2011-06-14 20:14:20 PDT
Okay, this implementation is totally different from the previous one, and we roll in a couple of other changes:

1) We cluster the tests so that we will run all of the tests in a directory before any tests in a subdirectory. This matters when we are only running parts or chunks of the whole suite.

2) We do natural sorting on the directory names as well as the path names. Switching to the key-based comparison turns >10lines of code into 2-3.

3) For some reason, old-run-webkit-tests will run tests in "foo-bar" before "foo" (notably in "inspector-enabled" before "inspector", but there's nothing special about the "-enabled" suffix, it just appends the directory separator onto the key before comparing.
Comment 16 Tony Chang 2011-06-16 11:43:36 PDT
Comment on attachment 97223 [details]
fix path sorting as well

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:284
> +        return (path[0:idx], path[idx:])

Shouldn't the second part be path[idx+1:] (i.e., without the slash)?  Also, do you want to handle the case of multiple slashes (e.g., foo//bar)?  Nit: You can omit the 0 in the first part of the tuple.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:374
> +            self._test_files_list.sort(key=lambda path: path_key(self._fs, path))

Nit: It might be a bit easier to read if path_key() is just a member function (then you don't have to use lambda).
Comment 17 Dirk Pranke 2011-06-16 14:21:11 PDT
Comment on attachment 97223 [details]
fix path sorting as well

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

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:284
>> +        return (path[0:idx], path[idx:])
> 
> Shouldn't the second part be path[idx+1:] (i.e., without the slash)?  Also, do you want to handle the case of multiple slashes (e.g., foo//bar)?  Nit: You can omit the 0 in the first part of the tuple.

You're right, good catch.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:374
>> +            self._test_files_list.sort(key=lambda path: path_key(self._fs, path))
> 
> Nit: It might be a bit easier to read if path_key() is just a member function (then you don't have to use lambda).

Yeah. I wanted to keep path_key as a standalone function so that it would be easier to test and be easier to relocate if we decided someplace else was a better home, though.
Comment 18 Dirk Pranke 2011-06-16 14:47:51 PDT
Created attachment 97505 [details]
update w/ feedback from tony, remove extraneous extra sort
Comment 19 WebKit Review Bot 2011-06-18 13:37:00 PDT
Comment on attachment 97505 [details]
update w/ feedback from tony, remove extraneous extra sort

Clearing flags on attachment: 97505

Committed r89206: <http://trac.webkit.org/changeset/89206>
Comment 20 WebKit Review Bot 2011-06-18 13:37:06 PDT
All reviewed patches have been landed.  Closing bug.