Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947603002:
Merge test results presentation of multiple shards. (Closed)
|
Created:
3 years, 6 months ago by BigBossZhiling Modified:
3 years, 6 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. |
DescriptionMerge test results presentation of multiple shards.
BUG=733824
Review-Url: https://codereview.chromium.org/2947603002
Cr-Commit-Position: refs/heads/master@{#481482}
Committed: https://chromium.googlesource.com/chromium/src/+/e82ecb088efcd8a4f8d28e6ff8b9185c2ba90f16
Patch Set 1 #
Total comments: 6
Patch Set 2 : address john's comments #
Total comments: 8
Patch Set 3 : address John's comments #
Total comments: 2
Patch Set 4 : address John's comments #
Messages
Total messages: 26 (9 generated)
hzl@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/merge_test.py (right): https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/merge_test.py:84: OUTPUT_JSON_SIZE_LIMIT = 100 * 1024 * 1024 # 100 MB Not sure whether we need this logic here. It is in the standard gtest merge function though.
https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/merge_test.py (right): https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/merge_test.py:1: #! /usr/bin/env python Name this file differently. _test.py implies that this is a test for something else. (You certainly don't have to keep standard_gtest_merge.py, but you could.) https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:15: from merge_test import merge_test We generally import modules, not the contents thereof. Also, for consistency, this should be from pylib.results.presentation import merge_test (% whatever new name you pick for merge_test) and should be down below w/ the other pylib import. https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:410: raise Exception('Not complying with merge API.') nit: this should say *why* the arguments aren't complying w/ the merge api.
https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/merge_test.py (right): https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/merge_test.py:1: #! /usr/bin/env python On 2017/06/19 13:33:42, jbudorick wrote: > Name this file differently. _test.py implies that this is a test for something > else. (You certainly don't have to keep standard_gtest_merge.py, but you could.) Done. https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/1/build/android/pylib/results... build/android/pylib/results/presentation/test_results_presentation.py:15: from merge_test import merge_test On 2017/06/19 13:33:42, jbudorick wrote: > We generally import modules, not the contents thereof. > > Also, for consistency, this should be > > from pylib.results.presentation import merge_test > > (% whatever new name you pick for merge_test) and should be down below w/ the > other pylib import. Done.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/standard_gtest_merge.py (right): https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/standard_gtest_merge.py:1: #! /usr/bin/env python Jumping in here, since Im curious and have some questions about this. If we are mergeing the JSON results, do we actually need the swarming merge interface for the results_presentation.py script? Since we can just run the results_presentation.py script with the merged JSON? Also, this script shouldnt be in presentation/ directory? right?
https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/standard_gtest_merge.py (right): https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/standard_gtest_merge.py:1: #! /usr/bin/env python Jumping in here, since Im curious and have some questions about this. If we are mergeing the JSON results, do we actually need the swarming merge interface for the results_presentation.py script? Since we can just run the results_presentation.py script with the merged JSON? Also, this script shouldnt be in presentation/ directory? right?
Correct me if I am wrong. Right now, after all tests are run in different shards, we will run test results presentation script. However whatever script we run, that script needs to have the merge API because the script needs to merge all test results. On Mon, Jun 19, 2017 at 11:36 AM <mikecase@chromium.org> wrote: > > > https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... > File build/android/pylib/results/presentation/standard_gtest_merge.py > (right): > > > https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... > build/android/pylib/results/presentation/standard_gtest_merge.py:1: #! > /usr/bin/env python > Jumping in here, since Im curious and have some questions about this. > > If we are mergeing the JSON results, do we actually need the swarming > merge interface for the results_presentation.py script? Since we can > just run the results_presentation.py script with the merged JSON? > > Also, this script shouldnt be in presentation/ directory? right? > > https://codereview.chromium.org/2947603002/ > -- 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 2017/06/19 18:53:29, chromium-reviews wrote: > Correct me if I am wrong. Right now, after all tests are run in different > shards, we will run test results presentation script. However whatever > script we run, that script needs to have the merge API because the script > needs to merge all test results. > > On Mon, Jun 19, 2017 at 11:36 AM <mailto:mikecase@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... > > File build/android/pylib/results/presentation/standard_gtest_merge.py > > (right): > > > > > > > https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... > > build/android/pylib/results/presentation/standard_gtest_merge.py:1: #! > > /usr/bin/env python > > Jumping in here, since Im curious and have some questions about this. > > > > If we are mergeing the JSON results, do we actually need the swarming > > merge interface for the results_presentation.py script? Since we can > > just run the results_presentation.py script with the merged JSON? There's no mechanism for doing this at the moment. We could conceivably modify the merge API to allow it, though I'm not particularly keen on doing so at the moment. > > > > Also, this script shouldnt be in presentation/ directory? right? > > > > https://codereview.chromium.org/2947603002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/standard_gtest_merge.py (right): https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/standard_gtest_merge.py:1: #! /usr/bin/env python On 2017/06/19 18:36:31, mikecase wrote: > Jumping in here, since Im curious and have some questions about this. > > If we are mergeing the JSON results, do we actually need the swarming merge > interface for the results_presentation.py script? Since we can just run the > results_presentation.py script with the merged JSON? > > Also, this script shouldnt be in presentation/ directory? right? (I responded to this above.) https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:22: from pylib.results.presentation import standard_gtest_merge nit: this should precede the google_storage_helper import https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:405: if not len(args.positional) == 1: nit: flip this conditional around to if len(args.positional) == 1: json_file = args.json_file else: ... https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:411: raise Exception('Arguments (output_json/summary_json) of' This was not what I meant. If args.output_json is missing, raise an exception saying that. If args.summary_json is missing, raise an exception saying that. The two do not have to be covered by the same exception.
https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:22: from pylib.results.presentation import standard_gtest_merge On 2017/06/21 21:05:06, jbudorick wrote: > nit: this should precede the google_storage_helper import Done. https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:405: if not len(args.positional) == 1: On 2017/06/21 21:05:05, jbudorick wrote: > nit: flip this conditional around to > > if len(args.positional) == 1: > json_file = args.json_file > else: > ... Done. https://codereview.chromium.org/2947603002/diff/20001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:411: raise Exception('Arguments (output_json/summary_json) of' On 2017/06/21 21:05:05, jbudorick wrote: > This was not what I meant. > > If args.output_json is missing, raise an exception saying that. If > args.summary_json is missing, raise an exception saying that. The two do not > have to be covered by the same exception. Done.
lgtm w/ nit https://codereview.chromium.org/2947603002/diff/40001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/40001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:412: else: nit: if args.output_json and args.summary_json: ... elif not args.output_json: raise ... else: raise ...
https://codereview.chromium.org/2947603002/diff/40001/build/android/pylib/res... File build/android/pylib/results/presentation/test_results_presentation.py (right): https://codereview.chromium.org/2947603002/diff/40001/build/android/pylib/res... build/android/pylib/results/presentation/test_results_presentation.py:412: else: On 2017/06/21 22:12:51, jbudorick wrote: > nit: > > if args.output_json and args.summary_json: > ... > elif not args.output_json: > raise ... > else: > raise ... Done.
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2947603002/#ps60001 (title: "address John's comments")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 60001, "attempt_start_ts": 1498116113375590,
"parent_rev": "7c65a26c058cf485c61809dfa70f8cc9d92d5560", "commit_rev":
"e82ecb088efcd8a4f8d28e6ff8b9185c2ba90f16"}
Message was sent while issue was closed.
Description was changed from ========== Merge test results presentation of multiple shards. BUG=733824 ========== to ========== Merge test results presentation of multiple shards. BUG=733824 Review-Url: https://codereview.chromium.org/2947603002 Cr-Commit-Position: refs/heads/master@{#481482} Committed: https://chromium.googlesource.com/chromium/src/+/e82ecb088efcd8a4f8d28e6ff8b9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e82ecb088efcd8a4f8d28e6ff8b9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
