Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453513002:
Change default behavior of --seed to be unix timestamp (Closed)
DescriptionCurrently if you pass --order=random but do not pass a --seed, the seed will default to 4. This changes the default seed to be time.time(). This matches the behavior of GTest and enables us to go ahead and deploy an FYI bot that runs the tests repeatedly in random order.
Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ
BUG=652357
Committed: https://crrev.com/57aa443c4cc4d1785692c8da7b8b17483c9e657a
Cr-Commit-Position: refs/heads/master@{#428407}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove new flag, change default seed to unix timestamp #
Total comments: 1
Patch Set 3 : Replace custom MockTime with host.time #
Total comments: 3
Patch Set 4 : Replace time mock with function-specific mock method #
Total comments: 4
Patch Set 5 : Address CL feedback from dpranke #
Messages
Total messages: 24 (7 generated)
Description was changed from ========== Introduce flag --seed-unixtime BUG=652357 ========== to ========== Introduce flag --seed-unixtime Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ==========
jeffcarp@chromium.org changed reviewers: + dpranke@chromium.org, ojan@chromium.org, qyearsley@chromium.org
Note, I'm a bit in favor of option 2 in https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ, but I think this is also OK :-) https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py (right): https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:378: self.assertNotEqual(tests_run_1, tests_run_2) If you really want to assert that the seed comes from time.time() and order is the same when the time is the same, then you could try to replace the time function with a stub that returns a canned value; maybe something like: original_time_fn = run_webkit_tests.time.time run_webkit_tests.time.time = lambda: 10 tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) self.assertEqual(tests_run_1, tests_run_2) run_webkit_tests.time.time = lambda: 20 tests_run_3 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) self.assertNotEqual(tests_run_3) run_webkit_tests.time.time = original_time_fn If you just want to exercise the code path and assert something basic like the same set of tests is run, you could do: tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) self.assertEqual(set(tests_run_1), set(tests_run_2))
Description was changed from ========== Introduce flag --seed-unixtime Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ========== to ========== Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ==========
Updated to remove the new flag and change the default behavior or seed to be the unix timestamp. https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py (right): https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:378: self.assertNotEqual(tests_run_1, tests_run_2) On 2016/10/25 at 19:21:39, qyearsley wrote: > If you really want to assert that the seed comes from time.time() and order is the same when the time is the same, then you could try to replace the time function with a stub that returns a canned value; maybe something like: > > original_time_fn = run_webkit_tests.time.time > run_webkit_tests.time.time = lambda: 10 > tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > self.assertEqual(tests_run_1, tests_run_2) > run_webkit_tests.time.time = lambda: 20 > tests_run_3 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > self.assertNotEqual(tests_run_3) > run_webkit_tests.time.time = original_time_fn > > If you just want to exercise the code path and assert something basic like the same set of tests is run, you could do: > > tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > self.assertEqual(set(tests_run_1), set(tests_run_2)) omg this is amazing... I had no idea you could mock things that easily in python. thank you!
On 2016/10/25 at 21:49:37, jeffcarp wrote: > Updated to remove the new flag and change the default behavior or seed to be the unix timestamp. > > https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py (right): > > https://codereview.chromium.org/2453513002/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:378: self.assertNotEqual(tests_run_1, tests_run_2) > On 2016/10/25 at 19:21:39, qyearsley wrote: > > If you really want to assert that the seed comes from time.time() and order is the same when the time is the same, then you could try to replace the time function with a stub that returns a canned value; maybe something like: > > > > original_time_fn = run_webkit_tests.time.time > > run_webkit_tests.time.time = lambda: 10 > > tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > > tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > > self.assertEqual(tests_run_1, tests_run_2) > > run_webkit_tests.time.time = lambda: 20 > > tests_run_3 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > > self.assertNotEqual(tests_run_3) > > run_webkit_tests.time.time = original_time_fn > > > > If you just want to exercise the code path and assert something basic like the same set of tests is run, you could do: > > > > tests_run_1 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > > tests_run_2 = get_tests_run(['--order=random', '--seed-unixtime'] + tests_to_run) > > self.assertEqual(set(tests_run_1), set(tests_run_2)) > > omg this is amazing... I had no idea you could mock things that easily in python. thank you! Aye :-) If one uses the mock module (https://docs.python.org/3/library/unittest.mock.html), I think that this could be a bit shorter... something like: def test_random_order_with_timestamp_seed(self): ... with mock.patch('time.time') as mock_time: mock_time.return_value = 10 ... mock_time.return_value = 20 ... ... Although in general, in webkitpy, the mock module isn't used, and fake classes (like MockFileSystem and MockWeb) are used instead wherever there are external dependencies. Anyway, it seems like there are several different ways that this could be tested, and I'm not sure which way is best; I think that this LGTM. Dirk or Ojan, do you have any thoughts about the unit test here?
https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py (right): https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: self.module.time.time = self.original_time_fn Do not do this. I consider stubbing out module functions to be a very strong anti-pattern in python testing code; if you end up needing to debug things it can be very confusing to tell what's going on, and it can make code fragile. 98% of the reasons that the Host and SystemHost classes exist is to provide wrappers around common libraries and system functions so that functions can be consistently overridden for testing purposes. You should add a time() method to the SystemHost (and MockSystemHost) classes, and call that instead.
On 2016/10/25 at 22:19:54, dpranke wrote: > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py (right): > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: self.module.time.time = self.original_time_fn > Do not do this. > > I consider stubbing out module functions to be a very strong anti-pattern in python testing code; if you end up needing to debug things it can be very confusing to tell what's going on, and it can make code fragile. > > 98% of the reasons that the Host and SystemHost classes exist is to provide wrappers around common libraries and system functions so that functions can be consistently overridden for testing purposes. > > You should add a time() method to the SystemHost (and MockSystemHost) classes, and call that instead. Good to know! I added it to SystemHost.
Please give a change description that describes the change. It's fine to also link to a discussion, but that doesn't replace the need for a good change description. As to the change itself, I have no opinion except that we should do whatever gtest does. The more we minimize unnecessary differences between test runners, the better. On Tue, Oct 25, 2016, 5:33 PM <jeffcarp@chromium.org> wrote: > On 2016/10/25 at 22:19:54, dpranke wrote: > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > File > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py > (right): > > > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: > self.module.time.time = self.original_time_fn > > Do not do this. > > > > I consider stubbing out module functions to be a very strong > anti-pattern in > python testing code; if you end up needing to debug things it can be very > confusing to tell what's going on, and it can make code fragile. > > > > 98% of the reasons that the Host and SystemHost classes exist is to > provide > wrappers around common libraries and system functions so that functions > can be > consistently overridden for testing purposes. > > > > You should add a time() method to the SystemHost (and MockSystemHost) > classes, > and call that instead. > > Good to know! I added it to SystemHost. > > https://codereview.chromium.org/2453513002/ > -- 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.
Please give a change description that describes the change. It's fine to also link to a discussion, but that doesn't replace the need for a good change description. As to the change itself, I have no opinion except that we should do whatever gtest does. The more we minimize unnecessary differences between test runners, the better. On Tue, Oct 25, 2016, 5:33 PM <jeffcarp@chromium.org> wrote: > On 2016/10/25 at 22:19:54, dpranke wrote: > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > File > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py > (right): > > > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: > self.module.time.time = self.original_time_fn > > Do not do this. > > > > I consider stubbing out module functions to be a very strong > anti-pattern in > python testing code; if you end up needing to debug things it can be very > confusing to tell what's going on, and it can make code fragile. > > > > 98% of the reasons that the Host and SystemHost classes exist is to > provide > wrappers around common libraries and system functions so that functions > can be > consistently overridden for testing purposes. > > > > You should add a time() method to the SystemHost (and MockSystemHost) > classes, > and call that instead. > > Good to know! I added it to SystemHost. > > https://codereview.chromium.org/2453513002/ > -- 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.
Will do. This change brings it in line with GTests's behavior (minus the magic number 0): https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > By default, Google Test uses a random seed calculated from the current time. Therefore you'll get a different order every time. The console output includes the random seed value, such that you can reproduce an order-related test failure later. To specify the random seed explicitly, use the --gtest_random_seed=SEED flag (or set the GTEST_RANDOM_SEED environment variable), where SEED is an integer between 0 and 99999. The seed value 0 is special: it tells Google Test to do the default behavior of calculating the seed from the current time. On Tue, Oct 25, 2016 at 7:36 PM Ojan Vafai <ojan@chromium.org> wrote: > Please give a change description that describes the change. It's fine to > also link to a discussion, but that doesn't replace the need for a good > change description. > > As to the change itself, I have no opinion except that we should do > whatever gtest does. The more we minimize unnecessary differences between > test runners, the better. > > On Tue, Oct 25, 2016, 5:33 PM <jeffcarp@chromium.org> wrote: > > On 2016/10/25 at 22:19:54, dpranke wrote: > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > File > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py > (right): > > > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: > self.module.time.time = self.original_time_fn > > Do not do this. > > > > I consider stubbing out module functions to be a very strong > anti-pattern in > python testing code; if you end up needing to debug things it can be very > confusing to tell what's going on, and it can make code fragile. > > > > 98% of the reasons that the Host and SystemHost classes exist is to > provide > wrappers around common libraries and system functions so that functions > can be > consistently overridden for testing purposes. > > > > You should add a time() method to the SystemHost (and MockSystemHost) > classes, > and call that instead. > > Good to know! I added it to SystemHost. > > https://codereview.chromium.org/2453513002/ > > -- 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.
Will do. This change brings it in line with GTests's behavior (minus the magic number 0): https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > By default, Google Test uses a random seed calculated from the current time. Therefore you'll get a different order every time. The console output includes the random seed value, such that you can reproduce an order-related test failure later. To specify the random seed explicitly, use the --gtest_random_seed=SEED flag (or set the GTEST_RANDOM_SEED environment variable), where SEED is an integer between 0 and 99999. The seed value 0 is special: it tells Google Test to do the default behavior of calculating the seed from the current time. On Tue, Oct 25, 2016 at 7:36 PM Ojan Vafai <ojan@chromium.org> wrote: > Please give a change description that describes the change. It's fine to > also link to a discussion, but that doesn't replace the need for a good > change description. > > As to the change itself, I have no opinion except that we should do > whatever gtest does. The more we minimize unnecessary differences between > test runners, the better. > > On Tue, Oct 25, 2016, 5:33 PM <jeffcarp@chromium.org> wrote: > > On 2016/10/25 at 22:19:54, dpranke wrote: > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > File > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py > (right): > > > > > > https://codereview.chromium.org/2453513002/diff/20001/third_party/WebKit/Tool... > > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:192: > self.module.time.time = self.original_time_fn > > Do not do this. > > > > I consider stubbing out module functions to be a very strong > anti-pattern in > python testing code; if you end up needing to debug things it can be very > confusing to tell what's going on, and it can make code fragile. > > > > 98% of the reasons that the Host and SystemHost classes exist is to > provide > wrappers around common libraries and system functions so that functions > can be > consistently overridden for testing purposes. > > > > You should add a time() method to the SystemHost (and MockSystemHost) > classes, > and call that instead. > > Good to know! I added it to SystemHost. > > https://codereview.chromium.org/2453513002/ > > -- 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.
Description was changed from ========== Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ========== to ========== Currently if you pass --order=random but do not pass a --seed, the seed will default to 4. This changes the default seed to be time.time(). This matches the behavior of GTest and enables us to go ahead and deploy an FYI bot that runs the tests repeatedly in random order. Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ==========
https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py (right): https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:50: self.time = time I'd provide a wrapper method for time.time() instead of a stub for the module. The former is simpler, and the latter exposes more capabilities than you need. https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:386: action="store", "store" is the default, so you don't need to specify it.
https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py (right): https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:50: self.time = time On 2016/10/26 at 17:24:51, Dirk Pranke (slow) wrote: > I'd provide a wrapper method for time.time() instead of a stub for the module. The former is > simpler, and the latter exposes more capabilities than you need. Great, updated.
On 2016/10/26 at 18:40:45, jeffcarp wrote: > https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py (right): > > https://codereview.chromium.org/2453513002/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:50: self.time = time > On 2016/10/26 at 17:24:51, Dirk Pranke (slow) wrote: > > I'd provide a wrapper method for time.time() instead of a stub for the module. The former is > > simpler, and the latter exposes more capabilities than you need. > > Great, updated. Ok I think this is ready for another review.
lgtm with the remaining comments addressed. https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right): https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:47: MockSystemHost.__init__(self, time_return_val, log_executive, executive_throws_when_run, Keep the parameters in the same order, i.e. move time_return_val to the end. https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py (right): https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py:61: def unixtime(): Don't use a @staticmethod, it doesn't really buy you anything. Also, I'd all this just "time". https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost_mock.py (right): https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost_mock.py:40: def __init__(self, time_return_val=123, log_executive=False, executive_throws_when_run=None, Move time_return_val to the end. https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2453513002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:386: action="store", store is the default, so you don't actually need this line, but it's not the end of the world to keep it, either.
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2453513002/#ps80001 (title: "Address CL feedback from dpranke")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Currently if you pass --order=random but do not pass a --seed, the seed will default to 4. This changes the default seed to be time.time(). This matches the behavior of GTest and enables us to go ahead and deploy an FYI bot that runs the tests repeatedly in random order. Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 ========== to ========== Currently if you pass --order=random but do not pass a --seed, the seed will default to 4. This changes the default seed to be time.time(). This matches the behavior of GTest and enables us to go ahead and deploy an FYI bot that runs the tests repeatedly in random order. Further discussion: https://groups.google.com/a/chromium.org/forum/#!topic/blink-infra/FL3Jdl_k_pQ BUG=652357 Committed: https://crrev.com/57aa443c4cc4d1785692c8da7b8b17483c9e657a Cr-Commit-Position: refs/heads/master@{#428407} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/57aa443c4cc4d1785692c8da7b8b17483c9e657a Cr-Commit-Position: refs/heads/master@{#428407} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
