Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821533005:
Make test results presentation enabled for swarming. (Closed)
|
Created:
3 years, 8 months ago by BigBossZhiling Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake test results presentation enabled for swarming.
In order to enable a script for swarming, a script has to enfore a
certain API. Therefore I am changing test_results_presentation to suite
this API.
BUG=605572
Review-Url: https://codereview.chromium.org/2821533005
Cr-Commit-Position: refs/heads/master@{#467064}
Committed: https://chromium.googlesource.com/chromium/src/+/a71da5150bbafbf33e18c97c837dd4f10dc39d49
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix john's comments #
Total comments: 8
Patch Set 3 : fixes #Patch Set 4 : fix arguments #
Total comments: 19
Patch Set 5 : fix comments #
Total comments: 7
Patch Set 6 : fixes #
Total comments: 1
Patch Set 7 : change recipes #Patch Set 8 : revert patch 7 and change loads to load #
Messages
Total messages: 32 (9 generated)
Description was changed from ========== Make test results presentation enabled for swarming. In order to enable a script for swarming, a script has to enfore a certain API. Therefore I am changing test_results_presentation to suite this API. BUG=605572 ========== to ========== Make test results presentation enabled for swarming. In order to enable a script for swarming, a script has to enfore a certain API. Therefore I am changing test_results_presentation to suite this API. BUG=605572 ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:329: help='(Swarming Isolated Merge Script API)' This should just be "(Swarming Merge Script API)", not "(Swarming Isolated Merge Script API)" https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:348: if (args.build_properties and args.output_json and Instead of splitting like this, handle these arguments individually. Load the build properties if they're passed, set the build number (1) if it's passed explicitly or (2) if it's in the build properties, etc. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:374: if is_swarming: Similarly, write the output json if output_json is passed to the script. https://codereview.chromium.org/2821533005/diff/1/testing/buildbot/chromium.l... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/2821533005/diff/1/testing/buildbot/chromium.l... testing/buildbot/chromium.linux.json:251: "https://storage.cloud.google.com", This is the server URL, right, not the bucket name?
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
s/enfore/enforce in commit desc https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:338: ' Summary of shard state running on swarming.' nit, you need a space at the end of this line or beginning of next. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:342: 'positional', nargs='*', Im not familiar with this swarming merge scripts. This accepts possibly many arguments but you only ever use args.positional[0]. Is this correct?
Good observation. John mentioned to me that for now there is only 1 argument. In the future there may be more, but currently reading from the first element is sufficient. On Mon, Apr 17, 2017 at 10:59 AM, <mikecase@chromium.org> wrote: > s/enfore/enforce in commit desc > > > https://codereview.chromium.org/2821533005/diff/1/build/ > android/pylib/results/presentation/test_results_presentation.py > File > build/android/pylib/results/presentation/test_results_presentation.py > (right): > > https://codereview.chromium.org/2821533005/diff/1/build/ > android/pylib/results/presentation/test_results_presentation.py#newcode338 > build/android/pylib/results/presentation/test_results_presentation.py:338: > ' Summary of shard state running on swarming.' > nit, you need a space at the end of this line or beginning of next. > > https://codereview.chromium.org/2821533005/diff/1/build/ > android/pylib/results/presentation/test_results_presentation.py#newcode342 > build/android/pylib/results/presentation/test_results_presentation.py:342: > 'positional', nargs='*', > Im not familiar with this swarming merge scripts. This accepts possibly > many arguments but you only ever use args.positional[0]. Is this > correct? > > https://codereview.chromium.org/2821533005/ > -- 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.
https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:329: help='(Swarming Isolated Merge Script API)' On 2017/04/17 13:00:46, jbudorick wrote: > This should just be "(Swarming Merge Script API)", not "(Swarming Isolated Merge > Script API)" Done. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:338: ' Summary of shard state running on swarming.' On 2017/04/17 17:59:20, mikecase wrote: > nit, you need a space at the end of this line or beginning of next. Done. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:342: 'positional', nargs='*', On 2017/04/17 17:59:20, mikecase wrote: > Im not familiar with this swarming merge scripts. This accepts possibly many > arguments but you only ever use args.positional[0]. Is this correct? Acknowledged. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:348: if (args.build_properties and args.output_json and On 2017/04/17 13:00:46, jbudorick wrote: > Instead of splitting like this, handle these arguments individually. Load the > build properties if they're passed, set the build number (1) if it's passed > explicitly or (2) if it's in the build properties, etc. Done. https://codereview.chromium.org/2821533005/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:374: if is_swarming: On 2017/04/17 13:00:46, jbudorick wrote: > Similarly, write the output json if output_json is passed to the script. Done. https://codereview.chromium.org/2821533005/diff/1/testing/buildbot/chromium.l... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/2821533005/diff/1/testing/buildbot/chromium.l... testing/buildbot/chromium.linux.json:251: "https://storage.cloud.google.com", On 2017/04/17 13:00:46, jbudorick wrote: > This is the server URL, right, not the bucket name? Done.
https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:348: if args.build_properties: As written, your argument logic makes a lot of unchecked assumptions that may result in this script silently doing something unexpected if a user passes in arguments in particular combinations. Instead of assuming that they won't do the wrong thing, we should check that they aren't and should fail hard if they do. Here, we're expecting either build_properties or both build_number and builder_name. https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:356: if args.output_json: Given how you're using args.output_json/output_json below, can you just omit these two lines and use args.output_json directly? https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:359: if args.positional: Here, we're expecting either json_file or positional, and we're expecting that positional is exactly one file. https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:365: if os.path.exists(json_file): If you're going to check this here and not just fail when we try to open it in result_details, you should rework this check from this: if good: do stuff else: error to this: if !good: error do stuff
https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:348: if args.build_properties: On 2017/04/17 20:40:04, jbudorick wrote: > As written, your argument logic makes a lot of unchecked assumptions that may > result in this script silently doing something unexpected if a user passes in > arguments in particular combinations. Instead of assuming that they won't do the > wrong thing, we should check that they aren't and should fail hard if they do. > > Here, we're expecting either build_properties or both build_number and > builder_name. Done. https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:356: if args.output_json: On 2017/04/17 20:40:04, jbudorick wrote: > Given how you're using args.output_json/output_json below, can you just omit > these two lines and use args.output_json directly? Done. https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:359: if args.positional: On 2017/04/17 20:40:04, jbudorick wrote: > Here, we're expecting either json_file or positional, and we're expecting that > positional is exactly one file. Done. https://codereview.chromium.org/2821533005/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:365: if os.path.exists(json_file): On 2017/04/17 20:40:04, jbudorick wrote: > If you're going to check this here and not just fail when we try to open it in > result_details, you should rework this check from this: > > if good: > do stuff > else: > error > > to this: > > if !good: > error > > do stuff Done.
https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:348: # One of build_perperties or (build_number or builder_names) should be given. nit: I would move these commnets inside the Exception message. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:350: bool(args.build_number or args.builder_name)): should this be bool(args.build_number AND args.builder_name)? https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:354: # or not given at all. nit: I would move these commnets inside the Exception message. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:358: # Only one of args.positional and args.json_file should be given. nit: I would move these commnets inside the Exception message.
https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:327: parser.add_argument( For future CLs, it's worth noting that argparse has the concept of mutually exclusive groups: https://docs.python.org/3/library/argparse.html#mutual-exclusion (I think the conditions we're trying to enforce here are a bit beyond the expressive capability of mutually exclusive groups, though.) https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:349: if not (bool(args.build_properties) ^ I would shy away from xor here and below. I think you could write this as if (args.build_properties is None) == (args.build_number is None or args.builder_name is None) https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:350: bool(args.build_number or args.builder_name)): On 2017/04/18 15:39:28, mikecase wrote: > should this be bool(args.build_number AND args.builder_name)? I think this part is OK as-is.= given the check below on build_number and builder_name. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:355: if bool(args.build_number) ^ bool(args.builder_name): if (args.build_number is None) != (args.builder_name is None): https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:359: if not bool(args.positional) ^ bool(args.json_file): (args.positional is None) != (args.json_file is None) https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:370: if args.build_number and args.builder_name: elif https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:379: if args.json_file: elif
https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:327: parser.add_argument( On 2017/04/18 15:59:25, jbudorick wrote: > For future CLs, it's worth noting that argparse has the concept of mutually > exclusive groups: > https://docs.python.org/3/library/argparse.html#mutual-exclusion > > (I think the conditions we're trying to enforce here are a bit beyond the > expressive capability of mutually exclusive groups, though.) Acknowledged. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:348: # One of build_perperties or (build_number or builder_names) should be given. On 2017/04/18 15:39:27, mikecase wrote: > nit: I would move these commnets inside the Exception message. Done. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:349: if not (bool(args.build_properties) ^ On 2017/04/18 15:59:25, jbudorick wrote: > I would shy away from xor here and below. I think you could write this as > > if (args.build_properties is None) == (args.build_number is None or > args.builder_name is None) Done. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:350: bool(args.build_number or args.builder_name)): On 2017/04/18 15:39:28, mikecase wrote: > should this be bool(args.build_number AND args.builder_name)? Acknowledged. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:358: # Only one of args.positional and args.json_file should be given. On 2017/04/18 15:39:27, mikecase wrote: > nit: I would move these commnets inside the Exception message. Done. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:359: if not bool(args.positional) ^ bool(args.json_file): On 2017/04/18 15:59:25, jbudorick wrote: > (args.positional is None) != (args.json_file is None) Acknowledged. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:370: if args.build_number and args.builder_name: On 2017/04/18 15:59:25, jbudorick wrote: > elif Done. https://codereview.chromium.org/2821533005/diff/60001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:379: if args.json_file: On 2017/04/18 15:59:25, jbudorick wrote: > elif Done.
https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:349: raise Exception('Exactly one of build_perperties or ' nit: switch all of these exceptions you're raising to parser.error(<message>) https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:392: json_object = json.loads(original_json_file.read()) nit: json.load(original_json_file) https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:395: f.write(str(json_object)) json.dump(json_object, f)
https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:349: raise Exception('Exactly one of build_perperties or ' On 2017/04/18 23:11:34, jbudorick wrote: > nit: switch all of these exceptions you're raising to parser.error(<message>) Done. https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:392: json_object = json.loads(original_json_file.read()) On 2017/04/18 23:11:34, jbudorick wrote: > nit: json.load(original_json_file) It has to be json.loads(<string>). Otherwise it will run into this question: Traceback (most recent call last): File "build/android/pylib/results/presentation/test_results_presentation.py", line 400, in <module> sys.exit(main()) File "build/android/pylib/results/presentation/test_results_presentation.py", line 392, in main json_object = json.loads(original_json_file) File "/usr/lib/python2.7/json/__init__.py", line 338, in loads return _default_decoder.decode(s) File "/usr/lib/python2.7/json/decoder.py", line 366, in decode obj, end = self.raw_decode(s, idx=_w(s, 0).end()) TypeError: expected string or buffer https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:395: f.write(str(json_object)) On 2017/04/18 23:11:34, jbudorick wrote: > json.dump(json_object, f) Done.
https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2821533005/diff/80001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:392: json_object = json.loads(original_json_file.read()) On 2017/04/18 23:35:54, BigBossZhiling wrote: > On 2017/04/18 23:11:34, jbudorick wrote: > > nit: json.load(original_json_file) > > It has to be json.loads(<string>). Otherwise it will run into this question: > Traceback (most recent call last): > File "build/android/pylib/results/presentation/test_results_presentation.py", > line 400, in <module> > sys.exit(main()) > File "build/android/pylib/results/presentation/test_results_presentation.py", > line 392, in main > json_object = json.loads(original_json_file) > File "/usr/lib/python2.7/json/__init__.py", line 338, in loads > return _default_decoder.decode(s) > File "/usr/lib/python2.7/json/decoder.py", line 366, in decode > obj, end = self.raw_decode(s, idx=_w(s, 0).end()) > TypeError: expected string or buffer json.load and json.loads are two different functions: https://docs.python.org/2/library/json.html#json.load
https://codereview.chromium.org/2821533005/diff/100001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/2821533005/diff/100001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:248: "merge": { This wasn't picked up. Looks like we need one more change to the recipes, this to pick up 'merge' from gtest specs.
lgtm :D
The CQ bit was checked by hzl@google.com
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hzl@google.com changed reviewers: + dpranke@chromium.org
On 2017/04/20 21:34:45, BigBossZhiling wrote: > mailto:hzl@google.com changed reviewers: > + mailto:dpranke@chromium.org dpranke: ptal at testing/buildbot/
lgtm
The CQ bit was checked by hzl@google.com
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": 140001, "attempt_start_ts": 1493140214983790,
"parent_rev": "bf4aa28fc62e2ad62111c2540deedcdbdde36749", "commit_rev":
"a71da5150bbafbf33e18c97c837dd4f10dc39d49"}
Message was sent while issue was closed.
Description was changed from ========== Make test results presentation enabled for swarming. In order to enable a script for swarming, a script has to enfore a certain API. Therefore I am changing test_results_presentation to suite this API. BUG=605572 ========== to ========== Make test results presentation enabled for swarming. In order to enable a script for swarming, a script has to enfore a certain API. Therefore I am changing test_results_presentation to suite this API. BUG=605572 Review-Url: https://codereview.chromium.org/2821533005 Cr-Commit-Position: refs/heads/master@{#467064} Committed: https://chromium.googlesource.com/chromium/src/+/a71da5150bbafbf33e18c97c837d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a71da5150bbafbf33e18c97c837d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
