Bug 58691 – new-run-webkit-tests: sort test files "correctly"

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

ORWT sorts the test files such that foo-2.html is run before foo-10.html. NRWT should match this.

[Dirk Pranke]

Created attachment 89848 [details]
Patch

[Eric Seidel (no email)]

I thought you had already posted a patch for this yesterday?

[Eric Seidel (no email)]

Comment on attachment 89848 [details]
Patch

Seems OK to me.

[Dirk Pranke]

(In reply to comment #2)
> I thought you had already posted a patch for this yesterday?

I did. Same patch, different bug.

[Tony Chang]

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.

[Dirk Pranke]

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.

[Tony Chang]

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))

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 89973 [details]
update w/ feedback from tony, restructure loop a bit

[Dirk Pranke]

Created attachment 95383 [details]
Patch

[Ryosuke Niwa]

Is this patch up for a review or not?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 97215 [details]
use a key= method instead of cmp=, rename to 'natural' sorting

[Dirk Pranke]

Created attachment 97223 [details]
fix path sorting as well

[Dirk Pranke]

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.

[Tony Chang]

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).

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 97505 [details]
update w/ feedback from tony, remove extraneous extra sort

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.