Created attachment 89848 [details] Patch

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

Comment on attachment 89848 [details] Patch Seems OK to me.

(In reply to comment #2) > I thought you had already posted a patch for this yesterday? I did. Same patch, different bug.

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

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

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

Created attachment 95383 [details] Patch

Is this patch up for a review or not?

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

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

Created attachment 97223 [details] fix path sorting as well

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

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

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>

All reviewed patches have been landed. Closing bug.