Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
890653002:
Add support for cc_perftests and other non-telemetry gtest based tests. (Closed)
DescriptionAdd support for cc_perftests and other non-telemetry gtest based tests.
This adds a src-side script for running non-telemetry based performance tests.
Will be followed by: https://codereview.chromium.org/873403002/
Example chromium.perf.json:
{
"Linux Perf (1)": {
"scripts": [
{
"name": "media_perftests",
"script": "perf_gtests.py",
"args": ["media_perftests", "--single-process-tests"]
},
{
"name": "load_library_perf_tests",
"script": "perf_gtests.py",
"args": ["load_library_perf_tests", "--single-process-tests"]
}
]
},
"Android Nexus7v2 Perf": {
"scripts": [
{
"name": "media_perftests",
"script": "perf_gtests.py",
"args": ["media_perftests"]
}
]
}
}
BUG=392620
Committed: https://crrev.com/9b700e91514b53e78aae3de7de730e6f31770df4
Cr-Commit-Position: refs/heads/master@{#318109}
Patch Set 1 : #Patch Set 2 : Emptied json file. #
Total comments: 13
Patch Set 3 : Changes from review. #
Total comments: 2
Patch Set 4 : Changes from review. #Patch Set 5 : Use log-processor-output-file. #Patch Set 6 : Use template string. #Patch Set 7 : Added _apk. #
Total comments: 7
Patch Set 8 : Added android_mode. #Patch Set 9 : Switched to target_platform. #
Messages
Total messages: 31 (9 generated)
Patchset #1 (id:1) has been deleted
simonhatch@chromium.org changed reviewers: + phajdan.jr@chromium.org
https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... File testing/scripts/perf_gtests.py (right): https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:1: #!/usr/bin/env python nit: Rename the file to start with "gtest_". We may have several gtest-related scripts here, and it seems more elegant for all of them to share a common prefix (as opposed to e.g. common suffix which may be slightly harder to notice). https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:50: 'valid': bool(rc <= common.MAX_FAILURES_EXIT_STATUS and rc == 0), Does gtest really use MAX_FAILURES_EXIT_STATUS? https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:51: 'failures': [], Sorry, this really *needs* to return actual list of failures. https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) This is suspicious - it seems this should request actual gtest target, rather than 'chrome'.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... File testing/scripts/perf_gtests.py (right): https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:1: #!/usr/bin/env python On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > nit: Rename the file to start with "gtest_". We may have several gtest-related > scripts here, and it seems more elegant for all of them to share a common prefix > (as opposed to e.g. common suffix which may be slightly harder to notice). Done. https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/30 12:16:52, Paweł Hajdan Jr. wrote: > nit: 2015 Done. https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:50: 'valid': bool(rc <= common.MAX_FAILURES_EXIT_STATUS and rc == 0), On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > Does gtest really use MAX_FAILURES_EXIT_STATUS? Yeah you're right, was just a leftover from copying telemetry_unittests. https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:51: 'failures': [], On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > Sorry, this really *needs* to return actual list of failures. There didn't seem to be any existing functionality to surface failures from runtest, so I've added that in the other CL. PTAL https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > This is suspicious - it seems this should request actual gtest target, rather > than 'chrome'. The perf bots don't actually build anything, so I just made this empty.
Thanks. Even though I'll be OOO next week, I'll _try_ to do another round of review. I strongly recommend you wait for my review, as things this CL is touching are very important API, that I'd rather keep in very good shape, rather than e.g. have to urgently fix it myself before people start coding against it. Still, please just use your best judgment here. https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... File testing/scripts/perf_gtests.py (right): https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) On 2015/01/30 16:46:52, shatch wrote: > On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > > This is suspicious - it seems this should request actual gtest target, rather > > than 'chrome'. > > The perf bots don't actually build anything, so I just made this empty. Sorry, I don't think it's correct. The chromium/chromium_trybot recipes can ensure that only builders compile, but the info about targets needs to be accurate. https://codereview.chromium.org/890653002/diff/120001/testing/scripts/gtest_p... File testing/scripts/gtest_perf_test.py (right): https://codereview.chromium.org/890653002/diff/120001/testing/scripts/gtest_p... testing/scripts/gtest_perf_test.py:56: 'failures': results.get('failed', []), results must not be empty; please remove the .get
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... File testing/scripts/perf_gtests.py (right): https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) On 2015/01/30 21:22:39, Paweł Hajdan Jr. (OOO) wrote: > On 2015/01/30 16:46:52, shatch wrote: > > On 2015/01/30 12:16:53, Paweł Hajdan Jr. wrote: > > > This is suspicious - it seems this should request actual gtest target, > rather > > > than 'chrome'. > > > > The perf bots don't actually build anything, so I just made this empty. > > Sorry, I don't think it's correct. The chromium/chromium_trybot recipes can > ensure that only builders compile, but the info about targets needs to be > accurate. Ok, I've included a list of supported gtests now. https://codereview.chromium.org/890653002/diff/120001/testing/scripts/gtest_p... File testing/scripts/gtest_perf_test.py (right): https://codereview.chromium.org/890653002/diff/120001/testing/scripts/gtest_p... testing/scripts/gtest_perf_test.py:56: 'failures': results.get('failed', []), On 2015/01/30 21:22:40, Paweł Hajdan Jr. (OOO) wrote: > results must not be empty; please remove the .get Done.
https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... File testing/scripts/perf_gtests.py (right): https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) On 2015/02/09 19:45:17, shatch wrote: > Ok, I've included a list of supported gtests now. I believe we'll need a more sophisticated solution than this. Let's see how this would apply to non-perf gtest tests. Since both base_unittests and browser_tests would be "supported tests", I'd have to compile browser_tests even if I'd only run base_unittests. The latter is very cheap to compile, which is really not true for browser_tests. My suggestion is to support variable substitutions in the result of compile targets call. For example, returning '${name}' would result in variable substituion inside ScriptTest (build/scripts/slave/recipe_modules/chromium/steps.py). This can be done e.g. using python template strings (https://docs.python.org/2/library/string.html#template-strings). Furthermore, this can handle os-specific cases like '${name}_apk' for Android.
On 2015/02/10 08:27:03, Paweł Hajdan Jr. (OOO) wrote: > https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... > File testing/scripts/perf_gtests.py (right): > > https://codereview.chromium.org/890653002/diff/40001/testing/scripts/perf_gte... > testing/scripts/perf_gtests.py:58: json.dump(['chrome'], args.output) > On 2015/02/09 19:45:17, shatch wrote: > > Ok, I've included a list of supported gtests now. > > I believe we'll need a more sophisticated solution than this. > > Let's see how this would apply to non-perf gtest tests. Since both > base_unittests and browser_tests would be "supported tests", I'd have to compile > browser_tests even if I'd only run base_unittests. The latter is very cheap to > compile, which is really not true for browser_tests. > > My suggestion is to support variable substitutions in the result of compile > targets call. For example, returning '${name}' would result in variable > substituion inside ScriptTest > (build/scripts/slave/recipe_modules/chromium/steps.py). This can be done e.g. > using python template strings > (https://docs.python.org/2/library/string.html#template-strings). Furthermore, > this can handle os-specific cases like '${name}_apk' for Android. Ok I took a pass at this, ptal at this CL and https://codereview.chromium.org/873403002/ where I actually do the name substition in ScriptTest::compile_targets.
phajdan.jr@chromium.org changed reviewers: + iannucci@chromium.org
Patchset 6 looks good to me. I'd prefer Robbie to also take a look.
On 2015/02/11 08:24:56, Paweł Hajdan Jr. (OOO) wrote: > Patchset 6 looks good to me. I'd prefer Robbie to also take a look. ping iannucci
On 2015/02/13 21:30:32, shatch wrote: > On 2015/02/11 08:24:56, Paweł Hajdan Jr. (OOO) wrote: > > Patchset 6 looks good to me. I'd prefer Robbie to also take a look. > > ping iannucci ping iannucci
https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.... testing/scripts/common.py:30: # TODO(phajdan.jr): Make build-config-fs required after passing it in recipe. TODO(phajdan.jr): help strings for these options so we can tell what they heck they do :) https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.... testing/scripts/common.py:34: parser.add_argument('--args', type=parse_json, default=[]) why is the introduction of a new argument necessary? Why can't the relevant info be shuttled through properties? https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p... File testing/scripts/gtest_perf_test.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p... testing/scripts/gtest_perf_test.py:36: if 'android' in perf_id: let's make this an explicit property (maybe 'android_mode'). The 'parse options from the value of other random properties' thing is cute, but we're not nearly consistent enough about it to make it non-surprising (sometimes it's mastername, sometimes buildername, sometimes perf-id, sometimes nothing, etc.). I think a separate property would be much clearer/more visible in invocations.
https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.... testing/scripts/common.py:34: parser.add_argument('--args', type=parse_json, default=[]) On 2015/02/23 18:02:49, iannucci wrote: > why is the introduction of a new argument necessary? Why can't the relevant info > be shuttled through properties? Actually... it looks like this isn't even used by the new script? AFAICT `args.properties` is actually referring to the --properties option above.
https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.... testing/scripts/common.py:34: parser.add_argument('--args', type=parse_json, default=[]) On 2015/02/23 18:04:33, iannucci wrote: > On 2015/02/23 18:02:49, iannucci wrote: > > why is the introduction of a new argument necessary? Why can't the relevant > info > > be shuttled through properties? > This was Pawel's preference when we spoke over email. > Actually... it looks like this isn't even used by the new script? AFAICT > `args.properties` is actually referring to the --properties option above. The new argument is used to get the script args. https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p... File testing/scripts/gtest_perf_test.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p... testing/scripts/gtest_perf_test.py:36: if 'android' in perf_id: On 2015/02/23 18:02:49, iannucci wrote: > let's make this an explicit property (maybe 'android_mode'). The 'parse options > from the value of other random properties' thing is cute, but we're not nearly > consistent enough about it to make it non-surprising (sometimes it's mastername, > sometimes buildername, sometimes perf-id, sometimes nothing, etc.). I think a > separate property would be much clearer/more visible in invocations. Done, added support for 'android_mode' in https://codereview.chromium.org/873403002/.
Okay. Could we add the rationale for another argument (as opposed to a property called 'script_args' or something) to this CL then? On Mon, Feb 23, 2015, 12:14 null <simonhatch@chromium.org> wrote: > > https://codereview.chromium.org/890653002/diff/220001/ > testing/scripts/common.py > File testing/scripts/common.py (right): > > https://codereview.chromium.org/890653002/diff/220001/ > testing/scripts/common.py#newcode34 > testing/scripts/common.py:34 > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common....>: > parser.add_argument('--args', > type=parse_json, default=[]) > On 2015/02/23 18:04:33, iannucci wrote: > > On 2015/02/23 18:02:49, iannucci wrote: > > > why is the introduction of a new argument necessary? Why can't the > relevant > > info > > > be shuttled through properties? > > > This was Pawel's preference when we spoke over email. > > > Actually... it looks like this isn't even used by the new script? > AFAICT > > `args.properties` is actually referring to the --properties option > above. > > The new argument is used to get the script args. > > https://codereview.chromium.org/890653002/diff/220001/ > testing/scripts/gtest_perf_test.py > File testing/scripts/gtest_perf_test.py (right): > > https://codereview.chromium.org/890653002/diff/220001/ > testing/scripts/gtest_perf_test.py#newcode36 > testing/scripts/gtest_perf_test.py:36 > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p...>: > if 'android' in perf_id: > On 2015/02/23 18:02:49, iannucci wrote: > > let's make this an explicit property (maybe 'android_mode'). The > 'parse options > > from the value of other random properties' thing is cute, but we're > not nearly > > consistent enough about it to make it non-surprising (sometimes it's > mastername, > > sometimes buildername, sometimes perf-id, sometimes nothing, etc.). I > think a > > separate property would be much clearer/more visible in invocations. > > Done, added support for 'android_mode' in > https://codereview.chromium.org/873403002/. > > https://codereview.chromium.org/890653002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.py File testing/scripts/common.py (right): https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common.... testing/scripts/common.py:34: parser.add_argument('--args', type=parse_json, default=[]) On 2015/02/23 20:14:21, shatch wrote: > On 2015/02/23 18:04:33, iannucci wrote: > > On 2015/02/23 18:02:49, iannucci wrote: > > > why is the introduction of a new argument necessary? Why can't the relevant > > info > > > be shuttled through properties? > > > > This was Pawel's preference when we spoke over email. > > > Actually... it looks like this isn't even used by the new script? AFAICT > > `args.properties` is actually referring to the --properties option above. > > The new argument is used to get the script args. I consider properties to be the same for all script invocations, and describe the environment of the build. Args can be different per invocation, e.g. you could have the same script but invoked in different steps with different args (e.g. a gtest step with different test names like base_unittests, net_unittests, etc). I think the distinction is important and actually results in cleaner code.
I don't really have any preference here, both seem to work just fine. AFAIK this wasn't much more than a mild preference on Pawel's side for a separate --args to indicate these parameters would be passed to the target script in this case. I'll write something to that effect. On 2015/02/23 20:16:24, iannucci wrote: > Okay. Could we add the rationale for another argument (as opposed to a > property called 'script_args' or something) to this CL then? > > On Mon, Feb 23, 2015, 12:14 null <mailto:simonhatch@chromium.org> wrote: > > > > > https://codereview.chromium.org/890653002/diff/220001/ > > testing/scripts/common.py > > File testing/scripts/common.py (right): > > > > https://codereview.chromium.org/890653002/diff/220001/ > > testing/scripts/common.py#newcode34 > > testing/scripts/common.py:34 > > > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common....>: > > parser.add_argument('--args', > > type=parse_json, default=[]) > > On 2015/02/23 18:04:33, iannucci wrote: > > > On 2015/02/23 18:02:49, iannucci wrote: > > > > why is the introduction of a new argument necessary? Why can't the > > relevant > > > info > > > > be shuttled through properties? > > > > > > This was Pawel's preference when we spoke over email. > > > > > Actually... it looks like this isn't even used by the new script? > > AFAICT > > > `args.properties` is actually referring to the --properties option > > above. > > > > The new argument is used to get the script args. > > > > https://codereview.chromium.org/890653002/diff/220001/ > > testing/scripts/gtest_perf_test.py > > File testing/scripts/gtest_perf_test.py (right): > > > > https://codereview.chromium.org/890653002/diff/220001/ > > testing/scripts/gtest_perf_test.py#newcode36 > > testing/scripts/gtest_perf_test.py:36 > > > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p...>: > > if 'android' in perf_id: > > On 2015/02/23 18:02:49, iannucci wrote: > > > let's make this an explicit property (maybe 'android_mode'). The > > 'parse options > > > from the value of other random properties' thing is cute, but we're > > not nearly > > > consistent enough about it to make it non-surprising (sometimes it's > > mastername, > > > sometimes buildername, sometimes perf-id, sometimes nothing, etc.). I > > think a > > > separate property would be much clearer/more visible in invocations. > > > > Done, added support for 'android_mode' in > > https://codereview.chromium.org/873403002/. > > > > https://codereview.chromium.org/890653002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Looks like Pawel answered just before I did, ignore my reply :) On 2015/02/23 20:47:39, shatch wrote: > I don't really have any preference here, both seem to work just fine. AFAIK this > wasn't much more than a mild preference on Pawel's side for a separate --args to > indicate these parameters would be passed to the target script in this case. > I'll write something to that effect. > > On 2015/02/23 20:16:24, iannucci wrote: > > Okay. Could we add the rationale for another argument (as opposed to a > > property called 'script_args' or something) to this CL then? > > > > On Mon, Feb 23, 2015, 12:14 null <mailto:simonhatch@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/890653002/diff/220001/ > > > testing/scripts/common.py > > > File testing/scripts/common.py (right): > > > > > > https://codereview.chromium.org/890653002/diff/220001/ > > > testing/scripts/common.py#newcode34 > > > testing/scripts/common.py:34 > > > > > > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/common....>: > > > parser.add_argument('--args', > > > type=parse_json, default=[]) > > > On 2015/02/23 18:04:33, iannucci wrote: > > > > On 2015/02/23 18:02:49, iannucci wrote: > > > > > why is the introduction of a new argument necessary? Why can't the > > > relevant > > > > info > > > > > be shuttled through properties? > > > > > > > > > This was Pawel's preference when we spoke over email. > > > > > > > Actually... it looks like this isn't even used by the new script? > > > AFAICT > > > > `args.properties` is actually referring to the --properties option > > > above. > > > > > > The new argument is used to get the script args. > > > > > > https://codereview.chromium.org/890653002/diff/220001/ > > > testing/scripts/gtest_perf_test.py > > > File testing/scripts/gtest_perf_test.py (right): > > > > > > https://codereview.chromium.org/890653002/diff/220001/ > > > testing/scripts/gtest_perf_test.py#newcode36 > > > testing/scripts/gtest_perf_test.py:36 > > > > > > <https://codereview.chromium.org/890653002/diff/220001/testing/scripts/gtest_p...>: > > > if 'android' in perf_id: > > > On 2015/02/23 18:02:49, iannucci wrote: > > > > let's make this an explicit property (maybe 'android_mode'). The > > > 'parse options > > > > from the value of other random properties' thing is cute, but we're > > > not nearly > > > > consistent enough about it to make it non-surprising (sometimes it's > > > mastername, > > > > sometimes buildername, sometimes perf-id, sometimes nothing, etc.). I > > > think a > > > > separate property would be much clearer/more visible in invocations. > > > > > > Done, added support for 'android_mode' in > > > https://codereview.chromium.org/873403002/. > > > > > > https://codereview.chromium.org/890653002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
kbr@chromium.org changed reviewers: + kbr@chromium.org
Could we please resolve the concerns on this CL and land it soon? Thanks.
LGTM
The CQ bit was checked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/890653002/260001
Message was sent while issue was closed.
Committed patchset #9 (id:260001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9b700e91514b53e78aae3de7de730e6f31770df4 Cr-Commit-Position: refs/heads/master@{#318109} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
