Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082653004:
blink/run-webkit-tests: Allow seeding the random test run order and write out value. (Closed)
Descriptionblink/run-webkit-tests: Allow seeding the random test run order and write out value.
Add "--order=random-seeded=10" to provide a seed to the random order. If no
seed is provided, then the default value of 4 continues to be used.
As the seed value can now be changed, it is written to a "random-seed.txt" file
in the output directory. The order the tests should be run is also written in
the "tests-run-order.txt" file.
BUG=601332, 524758
Patch Set 1 #Patch Set 2 : Fixing the test_run_order output. #Patch Set 3 : Fixing tests. #
Total comments: 6
Dependent Patchsets: Messages
Total messages: 21 (5 generated)
tansell@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
Hi, This CL adds support for controlling the seed value used in `--order=random-seeded`. It also writes the file the seed out to the test result directory (plus the test order) so it is recorded somewhere. Quinten is going to look further into http://crbug.com/601332 and needs this functionality to reproduce my results. This change is also needed to support the gtest API, a WIP version of that can be found at https://codereview.chromium.org/2086803002/ Tim 'mithro' Ansell
The CQ bit was checked by tansell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2082653004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:132: "\n".join(tests_to_run)) How does this relate to your "write a json file out" ? See also the comment I had in that CL that I'd just log the seed instead ...
Hi Dirk, Thanks for the quick review. I've tried to keep this change independent of the environment recording (http://crrev.com/2088713002/) and full gtest API CLs (https://codereview.chromium.org/2086803002/) as they both need further work / discussion and I'd like to allow Quinten (and his interns) to start work on reproduce my results regarding random test ordering of layout tests. Tim 'mithro' Ansell https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:132: "\n".join(tests_to_run)) On 2016/06/21 23:47:13, Dirk Pranke wrote: > How does this relate to your "write a json file out" ? Logging the random seed into the json environment file would be an alternative to logging to this file. I didn't want to block this patch on having the json environment recording figured out and a simple txt file seemed the simplest option. > See also the comment I had in that CL that I'd just log the seed instead ... I do log the random seed used on line 277. However, unless I'm missing something, the log output doesn't end up in the results output? As we are generating a lot of layout test results with different random seeds, it makes things a lot easier to have this information recorded in the output result somewhere. Happy to put it somewhere else if you have a better / more appropriate idea. The tests-run-order.txt was/is really useful when looking into these problems but it is less required. I can remove this bit (or put it behind a flag?) it if you prefer.
Thanks for uploading this :-) Along with this, the options help in run_webkit_tests.py should also be updated: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp... https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:268: rnd = random.Random() This line could be removed (it appears below). https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:270: groups = re.match('random-seeded(=([0-9]+))?', self._options.order) Instead of specifying seeds with an option like `--order=random-seeded=123`, what do you think about adding a separate option "--seed" which takes an integer? Then we also wouldn't need to have both --order=random and --order=random-seeded; whether it's using an input seed or generating a new seed could depend on whether --seed is passed in. For example: ... elif self._options.order == 'random': if self._options.seed is not None: seed = self._options.seed else: seed = int(time.time()) rnd = random.Random() test_to_run.sort() rnd.shuffle(tests_to_run) https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:274: seed = 4 # http://xkcd.com/221/ It's probably not necessary to necessary to keep this option here anymore; I don't think anything depends on using --order=random-seeded.
https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:270: groups = re.match('random-seeded(=([0-9]+))?', self._options.order) On 2016/06/22 17:30:16, qyearsley wrote: > Instead of specifying seeds with an option like `--order=random-seeded=123`, > what do you think about adding a separate option "--seed" which takes an > integer? Then we also wouldn't need to have both --order=random and > --order=random-seeded; whether it's using an input seed or generating a new seed > could depend on whether --seed is passed in. For example: > > ... > elif self._options.order == 'random': > if self._options.seed is not None: > seed = self._options.seed > else: > seed = int(time.time()) > rnd = random.Random() > test_to_run.sort() > rnd.shuffle(tests_to_run) > +1 to splitting it into a separate argument.
On 2016/06/24 at 01:53:40, dpranke wrote: > https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:270: groups = re.match('random-seeded(=([0-9]+))?', self._options.order) > On 2016/06/22 17:30:16, qyearsley wrote: > > Instead of specifying seeds with an option like `--order=random-seeded=123`, > > what do you think about adding a separate option "--seed" which takes an > > integer? Then we also wouldn't need to have both --order=random and > > --order=random-seeded; whether it's using an input seed or generating a new seed > > could depend on whether --seed is passed in. For example: > > > > ... > > elif self._options.order == 'random': > > if self._options.seed is not None: > > seed = self._options.seed > > else: > > seed = int(time.time()) > > rnd = random.Random() > > test_to_run.sort() > > rnd.shuffle(tests_to_run) > > > > +1 to splitting it into a separate argument. Ping -- I haven't started investigating order-dependent tests, but I think it would be good to add submit this change now anyway. Tim, does splitting --seed out to a separate argument sound good?
On 2016/07/20 23:24:16, qyearsley wrote: > On 2016/06/24 at 01:53:40, dpranke wrote: > > > https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... > > File > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py > (right): > > > > > https://codereview.chromium.org/2082653004/diff/40001/third_party/WebKit/Tool... > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:270: > groups = re.match('random-seeded(=([0-9]+))?', self._options.order) > > On 2016/06/22 17:30:16, qyearsley wrote: > > > Instead of specifying seeds with an option like `--order=random-seeded=123`, > > > what do you think about adding a separate option "--seed" which takes an > > > integer? Then we also wouldn't need to have both --order=random and > > > --order=random-seeded; whether it's using an input seed or generating a new > seed > > > could depend on whether --seed is passed in. For example: > > > > > > ... > > > elif self._options.order == 'random': > > > if self._options.seed is not None: > > > seed = self._options.seed > > > else: > > > seed = int(time.time()) > > > rnd = random.Random() > > > test_to_run.sort() > > > rnd.shuffle(tests_to_run) > > > > > > > +1 to splitting it into a separate argument. > > Ping -- I haven't started investigating order-dependent tests, but I think it > would be good to add submit this change now anyway. > > Tim, does splitting --seed out to a separate argument sound good? There are actually 3 different states here; 1) Use the current time as the random seed. (IE Order changes every time you run...) 2) Use a fixed default random seed (that isn't provided). 3) Use a fixed random seed that is provided. Currently, (1) is done via "--order=random", (2) is done via "--order=random-seeded" and (3) doesn't exist. If we want to with supporting the first two options then with a --seed option we would need to do; ---------- elif self._options.order.startswith('random'): if self._options.order.endswith('seeded'): if self._options.seed is not None: seed = int(self._options.seed) else: seed = 4 else: assert self._options.seed is not None, "--seed can only be used with random-seeded" seed = int(time.time()) ---------- I guess we could get rid of (2) and then your version works. Tim 'mithro' Ansell
tansell@chromium.org changed reviewers: + ojan@chromium.org
I somehow missed your mention of the fact that we should just get rid of the static random option. Dirk + Ojan, are you happy with removing "--order=random-seeded" and having "--order=random" be the seeded version which uses the "--seed" argument (and when not provided it uses the current time)? I'm assuming that nobody actually uses these arguments at the moment so won't complain if we change them? Tim 'mithro' Ansell
Yes, that makes sense. On Thu, Jul 21, 2016, 12:27 AM <tansell@chromium.org> wrote: > I somehow missed your mention of the fact that we should just get rid of > the > static random option. > > Dirk + Ojan, are you happy with removing "--order=random-seeded" and having > "--order=random" be the seeded version which uses the "--seed" argument > (and > when not provided it uses the current time)? > > I'm assuming that nobody actually uses these arguments at the moment so > won't > complain if we change them? > > Tim 'mithro' Ansell > > https://codereview.chromium.org/2082653004/ > -- 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.
Yes, that makes sense. On Thu, Jul 21, 2016, 12:27 AM <tansell@chromium.org> wrote: > I somehow missed your mention of the fact that we should just get rid of > the > static random option. > > Dirk + Ojan, are you happy with removing "--order=random-seeded" and having > "--order=random" be the seeded version which uses the "--seed" argument > (and > when not provided it uses the current time)? > > I'm assuming that nobody actually uses these arguments at the moment so > won't > complain if we change them? > > Tim 'mithro' Ansell > > https://codereview.chromium.org/2082653004/ > -- 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/07/21 16:18:40, ojan wrote: > Yes, that makes sense. Works for me, too.
On 2016/07/21 at 16:18:40, ojan wrote: > Yes, that makes sense. > > On Thu, Jul 21, 2016, 12:27 AM <tansell@chromium.org> wrote: > > > I somehow missed your mention of the fact that we should just get rid of > > the > > static random option. > > > > Dirk + Ojan, are you happy with removing "--order=random-seeded" and having > > "--order=random" be the seeded version which uses the "--seed" argument > > (and > > when not provided it uses the current time)? > > > > I'm assuming that nobody actually uses these arguments at the moment so > > won't > > complain if we change them? > > > > Tim 'mithro' Ansell Hi Tim, sounds like nobody would complain if you removed those options; if you have time, could you finish up and commit this CL?
Description was changed from ========== blink/run-webkit-tests: Allow seeding the random test run order and write out value. Add "--order=random-seeded=10" to provide a seed to the random order. If no seed is provided, then the default value of 4 continues to be used. As the seed value can now be changed, it is written to a "random-seed.txt" file in the output directory. The order the tests should be run is also written in the "tests-run-order.txt" file. BUG=601332, 524758 ========== to ========== blink/run-webkit-tests: Allow seeding the random test run order and write out value. Add "--order=random-seeded=10" to provide a seed to the random order. If no seed is provided, then the default value of 4 continues to be used. As the seed value can now be changed, it is written to a "random-seed.txt" file in the output directory. The order the tests should be run is also written in the "tests-run-order.txt" file. BUG=601332, 524758 ==========
On 2016/08/18 at 21:45:36, qyearsley wrote: > On 2016/07/21 at 16:18:40, ojan wrote: > > Yes, that makes sense. > > > > On Thu, Jul 21, 2016, 12:27 AM <tansell@chromium.org> wrote: > > > > > I somehow missed your mention of the fact that we should just get rid of > > > the > > > static random option. > > > > > > Dirk + Ojan, are you happy with removing "--order=random-seeded" and having > > > "--order=random" be the seeded version which uses the "--seed" argument > > > (and > > > when not provided it uses the current time)? > > > > > > I'm assuming that nobody actually uses these arguments at the moment so > > > won't > > > complain if we change them? > > > > > > Tim 'mithro' Ansell > > Hi Tim, sounds like nobody would complain if you removed those options; if you have time, could you finish up and commit this CL? Hi, this looks useful for finding tests with order dependencies. Are you planning to finish this CL soon or would you mind if qyearsley and I picked it up?
On 2016/08/31 at 17:16:07, jeffcarp wrote: > On 2016/08/18 at 21:45:36, qyearsley wrote: > > On 2016/07/21 at 16:18:40, ojan wrote: > > > Yes, that makes sense. > > > > > > On Thu, Jul 21, 2016, 12:27 AM <tansell@chromium.org> wrote: > > > > > > > I somehow missed your mention of the fact that we should just get rid of > > > > the > > > > static random option. > > > > > > > > Dirk + Ojan, are you happy with removing "--order=random-seeded" and having > > > > "--order=random" be the seeded version which uses the "--seed" argument > > > > (and > > > > when not provided it uses the current time)? > > > > > > > > I'm assuming that nobody actually uses these arguments at the moment so > > > > won't > > > > complain if we change them? > > > > > > > > Tim 'mithro' Ansell > > > > Hi Tim, sounds like nobody would complain if you removed those options; if you have time, could you finish up and commit this CL? > > Hi, this looks useful for finding tests with order dependencies. Are you planning to finish this CL soon or would you mind if qyearsley and I picked it up? Made a continuation of this CL that removes --random-seeded and makes --seed a separate option: http://crrev.com/2308283002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
