Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557673003:
Change run_webkit_tests default order to random on Linux and Mac (Closed)
DescriptionChange run_webkit_tests default order to random on Linux and Mac
This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order.
BUG=671805
R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org
Committed: https://crrev.com/cf4cbd7b38be23f7bcf3aab5ed3a448f5bc6dd5d
Cr-Commit-Position: refs/heads/master@{#439210}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Default to random order only on Mac and Linux #Patch Set 3 : Add unit test for random order default on some platforms #Patch Set 4 : Attempt to fix unit test failures on Mac & Windows by specifying order #
Messages
Total messages: 43 (13 generated)
Description was changed from ========== Change run_webkit_tests default order to random BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org ========== to ========== Change run_webkit_tests default order to random If everything is OK, I plan on landing this on December 14 when we make the switch to random by default on the CQ. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org ==========
lgtm!
What, if any, is the impact of this on cycle time?
On 2016/12/07 at 01:04:31, dpranke wrote: > What, if any, is the impact of this on cycle time? We've observed that the RandomOrder builders are consistently 1-2 minutes faster than the default order builders. However, there might be other reasons behind that like hardware differences.
On 2016/12/07 01:07:23, jeffcarp wrote: > On 2016/12/07 at 01:04:31, dpranke wrote: > > What, if any, is the impact of this on cycle time? > > We've observed that the RandomOrder builders are consistently 1-2 minutes faster > than the default order builders. However, there might be other reasons behind > that like hardware differences. Can you run some tests locally (or flip the switch back and forth on the builders) to confirm that and rule out hardware differences?
On 2016/12/07 at 01:29:21, dpranke wrote: > On 2016/12/07 01:07:23, jeffcarp wrote: > > On 2016/12/07 at 01:04:31, dpranke wrote: > > > What, if any, is the impact of this on cycle time? > > > > We've observed that the RandomOrder builders are consistently 1-2 minutes faster > > than the default order builders. However, there might be other reasons behind > > that like hardware differences. > > Can you run some tests locally (or flip the switch back and forth on the builders) to confirm that > and rule out hardware differences? I can run the tests on my workstation overnight tonight to get some data points.
On 2016/12/07 at 18:40:38, jeffcarp wrote: > On 2016/12/07 at 01:29:21, dpranke wrote: > > On 2016/12/07 01:07:23, jeffcarp wrote: > > > On 2016/12/07 at 01:04:31, dpranke wrote: > > > > What, if any, is the impact of this on cycle time? > > > > > > We've observed that the RandomOrder builders are consistently 1-2 minutes faster > > > than the default order builders. However, there might be other reasons behind > > > that like hardware differences. > > > > Can you run some tests locally (or flip the switch back and forth on the builders) to confirm that > > and rule out hardware differences? > > I can run the tests on my workstation overnight tonight to get some data points. Results from 46 test runs (23 random, 23 default): - Random mean time 25.40 minutes - Default mean time 25.62 minutes They are both within a standard deviation (0.53 & 0.36) of each other so these numbers don't make it seem like there's any added benefit of running in random order :(
Have you looked at number of tested retried? Might be that there's flake hidden by retries, which in turn slows down the test run. On Fri, Dec 9, 2016, 2:13 PM <jeffcarp@chromium.org> wrote: > On 2016/12/07 at 18:40:38, jeffcarp wrote: > > On 2016/12/07 at 01:29:21, dpranke wrote: > > > On 2016/12/07 01:07:23, jeffcarp wrote: > > > > On 2016/12/07 at 01:04:31, dpranke wrote: > > > > > What, if any, is the impact of this on cycle time? > > > > > > > > We've observed that the RandomOrder builders are consistently 1-2 > minutes > faster > > > > than the default order builders. However, there might be other > reasons > behind > > > > that like hardware differences. > > > > > > Can you run some tests locally (or flip the switch back and forth on > the > builders) to confirm that > > > and rule out hardware differences? > > > > I can run the tests on my workstation overnight tonight to get some data > points. > > Results from 46 test runs (23 random, 23 default): > > - Random mean time 25.40 minutes > - Default mean time 25.62 minutes > > They are both within a standard deviation (0.53 & 0.36) of each other so > these > numbers don't make it seem like there's any added benefit of running in > random > order :( > > https://codereview.chromium.org/2557673003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Have you looked at number of tested retried? Might be that there's flake hidden by retries, which in turn slows down the test run. On Fri, Dec 9, 2016, 2:13 PM <jeffcarp@chromium.org> wrote: > On 2016/12/07 at 18:40:38, jeffcarp wrote: > > On 2016/12/07 at 01:29:21, dpranke wrote: > > > On 2016/12/07 01:07:23, jeffcarp wrote: > > > > On 2016/12/07 at 01:04:31, dpranke wrote: > > > > > What, if any, is the impact of this on cycle time? > > > > > > > > We've observed that the RandomOrder builders are consistently 1-2 > minutes > faster > > > > than the default order builders. However, there might be other > reasons > behind > > > > that like hardware differences. > > > > > > Can you run some tests locally (or flip the switch back and forth on > the > builders) to confirm that > > > and rule out hardware differences? > > > > I can run the tests on my workstation overnight tonight to get some data > points. > > Results from 46 test runs (23 random, 23 default): > > - Random mean time 25.40 minutes > - Default mean time 25.62 minutes > > They are both within a standard deviation (0.53 & 0.36) of each other so > these > numbers don't make it seem like there's any added benefit of running in > random > order :( > > https://codereview.chromium.org/2557673003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/09 22:13:26, jeffcarp wrote: > Results from 46 test runs (23 random, 23 default): > > - Random mean time 25.40 minutes > - Default mean time 25.62 minutes > > They are both within a standard deviation (0.53 & 0.36) of each other so these > numbers don't make it seem like there's any added benefit of running in random > order :( On the contrary, I'm pleased by this as I would've expected the tests to be slower when run in random order.
lgtm Before submitting this, I think we wanted to change it to not affect Windows yet, right?
https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: default=("natural" if sys.platform == "win32" else "random") Or, more verbosely but not requiring importing sys: default=("natural" if self.host.port_factory.get().port_name == "win" else "random") And add a TODO note making a reference to crbug.com/601332. https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:336: "'natural' == use the natural order (default), " This help text should be updated.
https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", On 2016/12/14 at 17:58:21, qyearsley wrote: > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > default=("natural" if sys.platform == "win32" else "random") > > Or, more verbosely but not requiring importing sys: > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > And add a TODO note making a reference to crbug.com/601332. Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy.
On 2016/12/14 at 18:07:46, jeffcarp wrote: > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > On 2016/12/14 at 17:58:21, qyearsley wrote: > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > default=("natural" if sys.platform == "win32" else "random") > > > > Or, more verbosely but not requiring importing sys: > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > And add a TODO note making a reference to crbug.com/601332. > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being.
On 2016/12/14 at 18:10:21, jeffcarp wrote: > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.)
On 2016/12/14 at 18:14:06, qyearsley wrote: > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage.
On 2016/12/14 at 18:55:44, jeffcarp wrote: > On 2016/12/14 at 18:14:06, qyearsley wrote: > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. Ok unit tests added. Lmk what you think.
On 2016/12/14 at 22:02:15, jeffcarp wrote: > On 2016/12/14 at 18:55:44, jeffcarp wrote: > > On 2016/12/14 at 18:14:06, qyearsley wrote: > > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. > > Ok unit tests added. Lmk what you think. LGTM, it looks like this will do what we want :-) Later when the order is switched to random by default for all platforms, then that test method can be removed and the code can be simplified.
Description was changed from ========== Change run_webkit_tests default order to random If everything is OK, I plan on landing this on December 14 when we make the switch to random by default on the CQ. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org ========== to ========== Change run_webkit_tests default order to random on Linux and Mac This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org ==========
On 2016/12/14 at 22:09:20, qyearsley wrote: > On 2016/12/14 at 22:02:15, jeffcarp wrote: > > On 2016/12/14 at 18:55:44, jeffcarp wrote: > > > On 2016/12/14 at 18:14:06, qyearsley wrote: > > > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > > > > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > > > > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. > > > > Ok unit tests added. Lmk what you think. > > LGTM, it looks like this will do what we want :-) Later when the order is switched to random by default for all platforms, then that test method can be removed and the code can be simplified. Since running in random order requires RandomOrderExpectations, how do you feel about merging RandomOrderExpectations into TestExpectations in this CL?
On 2016/12/14 at 22:58:28, jeffcarp wrote: > On 2016/12/14 at 22:09:20, qyearsley wrote: > > On 2016/12/14 at 22:02:15, jeffcarp wrote: > > > On 2016/12/14 at 18:55:44, jeffcarp wrote: > > > > On 2016/12/14 at 18:14:06, qyearsley wrote: > > > > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > > > > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > > > > > > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > > > > > > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. > > > > > > Ok unit tests added. Lmk what you think. > > > > LGTM, it looks like this will do what we want :-) Later when the order is switched to random by default for all platforms, then that test method can be removed and the code can be simplified. > > Since running in random order requires RandomOrderExpectations, how do you feel about merging RandomOrderExpectations into TestExpectations in this CL? No need to add it in this CL, I think; that could be made into a separate CL and landed separately. Also, I don't think it would be a real problem, but that would also add some [ Pass Failure ] test expectations that would also apply to Windows and Android immediately, even those would still be running in "natural" order by default.
On 2016/12/14 at 23:05:12, qyearsley wrote: > On 2016/12/14 at 22:58:28, jeffcarp wrote: > > On 2016/12/14 at 22:09:20, qyearsley wrote: > > > On 2016/12/14 at 22:02:15, jeffcarp wrote: > > > > On 2016/12/14 at 18:55:44, jeffcarp wrote: > > > > > On 2016/12/14 at 18:14:06, qyearsley wrote: > > > > > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > > > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > > > > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > > > > > > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > > > > > > > > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > > > > > > > > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. > > > > > > > > Ok unit tests added. Lmk what you think. > > > > > > LGTM, it looks like this will do what we want :-) Later when the order is switched to random by default for all platforms, then that test method can be removed and the code can be simplified. > > > > Since running in random order requires RandomOrderExpectations, how do you feel about merging RandomOrderExpectations into TestExpectations in this CL? > > No need to add it in this CL, I think; that could be made into a separate CL and landed separately. > > Also, I don't think it would be a real problem, but that would also add some [ Pass Failure ] test expectations that would also apply to Windows and Android immediately, even those would still be running in "natural" order by default. True, I haven't been able to figure out the correct syntax for only including Windows. Looking at TestExpectations, it looks like what we had before, '[ Win ]', is right, but that wasn't working on the RandomOrder Win builder. Is there a chance TestExpectations and RandomOrderExpectations are parsed differently? I'll make a CL to merge RandomOrderExpectations into TestExpectations and land it before this one. That will mean some tests will start losing coverage immediately after it's landed. https://bugs.chromium.org/p/chromium/issues/detail?id=671802 still hasn't been sorted out, so I'm not sure what the best course of action is to maintain coverage.
On 2016/12/14 at 23:10:27, jeffcarp wrote: > On 2016/12/14 at 23:05:12, qyearsley wrote: > > On 2016/12/14 at 22:58:28, jeffcarp wrote: > > > On 2016/12/14 at 22:09:20, qyearsley wrote: > > > > On 2016/12/14 at 22:02:15, jeffcarp wrote: > > > > > On 2016/12/14 at 18:55:44, jeffcarp wrote: > > > > > > On 2016/12/14 at 18:14:06, qyearsley wrote: > > > > > > > On 2016/12/14 at 18:10:21, jeffcarp wrote: > > > > > > > > On 2016/12/14 at 18:07:46, jeffcarp wrote: > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2557673003/diff/1/third_party/WebKit/Tools/Sc... > > > > > > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:332: default="random", > > > > > > > > > On 2016/12/14 at 17:58:21, qyearsley wrote: > > > > > > > > > > In order to temporarily keep it as "natural" by default on Windows, you could change this to something like: > > > > > > > > > > > > > > > > > > > > default=("natural" if sys.platform == "win32" else "random") > > > > > > > > > > > > > > > > > > > > Or, more verbosely but not requiring importing sys: > > > > > > > > > > > > > > > > > > > > default=("natural" if self.host.port_factory.get().port_name == "win" else "random") > > > > > > > > > > > > > > > > > > > > And add a TODO note making a reference to crbug.com/601332. > > > > > > > > > > > > > > > > > > Thank you so much for this!! I was worrying about how to find out how to filter by platform in webkitpy. > > > > > > > > > > > > > > > > Also I think it'd make more sense to include Mac & Linux instead of excluding Windows, since we also want to exclude Android from random order for the time being. > > > > > > > > > > > > > > Makes sense. (Quick thought about formatting: if that part of the code is a bit cramped, we could also define a order_default variable above, which would give a bit more space for a comment and a normal if/else branch.) > > > > > > > > > > > > Uploaded a new patch for what we discussed above. Unfortunately I don't think we can do this in parse_args because host doesn't exist yet there. Let me know if this works. > > > > > > > > > > > > Also I was having trouble unit testing this. Kept running into 'AssertionError: LayoutTests/VirtualTestSuites not found' when trying to mock port_obj. Will keep picking away at that since this change should have test coverage. > > > > > > > > > > Ok unit tests added. Lmk what you think. > > > > > > > > LGTM, it looks like this will do what we want :-) Later when the order is switched to random by default for all platforms, then that test method can be removed and the code can be simplified. > > > > > > Since running in random order requires RandomOrderExpectations, how do you feel about merging RandomOrderExpectations into TestExpectations in this CL? > > > > No need to add it in this CL, I think; that could be made into a separate CL and landed separately. > > > > Also, I don't think it would be a real problem, but that would also add some [ Pass Failure ] test expectations that would also apply to Windows and Android immediately, even those would still be running in "natural" order by default. > > True, I haven't been able to figure out the correct syntax for only including Windows. Looking at TestExpectations, it looks like what we had before, '[ Win ]', is right, but that wasn't working on the RandomOrder Win builder. Is there a chance TestExpectations and RandomOrderExpectations are parsed differently? > > I'll make a CL to merge RandomOrderExpectations into TestExpectations and land it before this one. That will mean some tests will start losing coverage immediately after it's landed. https://bugs.chromium.org/p/chromium/issues/detail?id=671802 still hasn't been sorted out, so I'm not sure what the best course of action is to maintain coverage. > some tests will start losing coverage immediately after it's landed Giving this more thought, this is what we said would happen in the announcement email. The question I have now is whether it's ok to go ahead with the merge now and potentially have 1-2 days where those tests aren't getting coverage as we transition the RandomOrder builders to be DefaultOrder builders.
lgtm. I'm fine w/ losing coverage for a couple days.
On 2016/12/15 at 02:41:40, dpranke wrote: > lgtm. I'm fine w/ losing coverage for a couple days. Sounds good. In terms of where we are with the Linux & Mac switch: 1. https://crrev.com/2575293002 needs to be reviewed and landed (bug: https://crbug.com/674313) 2. Coordinating with a trooper, this CL needs to be landed (bug: https://crbug.com/671805) 3. Monitor builds & respond to anything that comes up 4. Convert the RandomOrder Linux & Mac builders to DefaultOrder (bug: https://crbug.com/671802) After Linux & Mac stabilize I'll work on landing Windows.
The CQ bit was checked by jeffcarp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jeffcarp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/16 at 20:03:48, commit-bot wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Update on this CL: there were unit test failures I didn't anticipate on Mac & Windows (changing the default test order caused a ton of run_webkit_tests_unittests to fail). I uploaded a patch that specified ['--order', 'natural'] on all affected tests and set off a CQ dry run.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2557673003/#ps60001 (title: "Attempt to fix unit test failures on Mac & Windows by specifying order")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481925626400020,
"parent_rev": "e831fff1aa84b6d23b48083c5d8c8ef65801dbf4", "commit_rev":
"5480d9f84608632ef3b654115798209368e7d795"}
Message was sent while issue was closed.
Description was changed from ========== Change run_webkit_tests default order to random on Linux and Mac This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org ========== to ========== Change run_webkit_tests default order to random on Linux and Mac This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org Review-Url: https://codereview.chromium.org/2557673003 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Change run_webkit_tests default order to random on Linux and Mac This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org Review-Url: https://codereview.chromium.org/2557673003 ========== to ========== Change run_webkit_tests default order to random on Linux and Mac This is the first step in moving all platforms to random order. Once Windows is stable in random order I will follow this up with a CL that removes the platform-specific logic and leaves default='random' for --order. BUG=671805 R=dpranke@chromium.org,qyearsley@chromium.org,ojan@chromium.org Committed: https://crrev.com/cf4cbd7b38be23f7bcf3aab5ed3a448f5bc6dd5d Cr-Commit-Position: refs/heads/master@{#439210} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cf4cbd7b38be23f7bcf3aab5ed3a448f5bc6dd5d Cr-Commit-Position: refs/heads/master@{#439210} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
