Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649523003:
[Predator] Add maximum number argument to delta test. (Closed)
|
Created:
3 years, 11 months ago by Sharu Jiang Modified:
3 years, 11 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, lijeffrey Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Add maximum number argument to delta test.
So the delta test can do something like "python run-delta-test.py -m 20" to check at most 20 crashes.
BUG=
Review-Url: https://codereview.chromium.org/2649523003
Committed: https://chromium.googlesource.com/infra/infra/+/54378e43b4346da07068052f91247c43e5f9100e
Patch Set 1 : Rebase. #
Total comments: 5
Patch Set 2 : Fix nits. #
Total comments: 4
Patch Set 3 : Fix nits. #
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Add maximum number argument to delta test. BUG= ========== to ========== [Predator] Add maximum number argument to delta test. BUG= ==========
katesonia@chromium.org changed reviewers: + chanli@chromium.org, inferno@chromium.org, mbarbella@chromium.org
PTAL.
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Predator] Add maximum number argument to delta test. BUG= ========== to ========== [Predator] Add maximum number argument to delta test. So the delta test can do something like "python run-delta-test.py -m 20" to check at most 20 crashes. BUG= ==========
https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py (right): https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:225: # Trucates the crashes and make it contains at most max_n crashes. nit: Truncates https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:241: crash_count += len(crashes) If has truncated above, crash_count should equal to max_n, right?
https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py (right): https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:225: # Trucates the crashes and make it contains at most max_n crashes. On 2017/01/23 21:59:38, chanli wrote: > nit: Truncates Done. https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:241: crash_count += len(crashes) On 2017/01/23 21:59:38, chanli wrote: > If has truncated above, crash_count should equal to max_n, right? Yes.
On 2017/01/24 01:59:39, Sharu Jiang wrote: > https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... > File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py > (right): > > https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... > appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:225: # > Trucates the crashes and make it contains at most max_n crashes. > On 2017/01/23 21:59:38, chanli wrote: > > nit: Truncates > > Done. > > https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... > appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:241: > crash_count += len(crashes) > On 2017/01/23 21:59:38, chanli wrote: > > If has truncated above, crash_count should equal to max_n, right? > > Yes. lgtm
lijeffrey@chromium.org changed reviewers: + lijeffrey@chromium.org
lgtm with nits https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py (right): https://codereview.chromium.org/2649523003/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:202: max_n: (int): Maximum total number of crashes. nit: why not just give this the full name max_number_of_crashes? https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py (right): https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:225: # Truncates the crashes and make it contains at most max_n crashes. nit: Truncate crashes and make it contain at most max_n crashes. https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py (right): https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py:146: args.batch = args.max if args.max < args.batch else args.batch how about args.batch = min(args.max, args.batch)?
lgtm once open comments are addressed
https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py (right): https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/delta_test.py:225: # Truncates the crashes and make it contains at most max_n crashes. On 2017/01/24 02:22:49, lijeffrey wrote: > nit: Truncate crashes and make it contain at most max_n crashes. Done. https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... File appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py (right): https://codereview.chromium.org/2649523003/diff/40001/appengine/findit/util_s... appengine/findit/util_scripts/crash_queries/delta_test/run-delta-test.py:146: args.batch = args.max if args.max < args.batch else args.batch On 2017/01/24 02:22:49, lijeffrey wrote: > how about > args.batch = min(args.max, args.batch)? Done.
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, mbarbella@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2649523003/#ps60001 (title: "Fix nits.")
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": 1485372410346130,
"parent_rev": "74acc18d3328055db9bb4fc5174d683c9d906e90", "commit_rev":
"54378e43b4346da07068052f91247c43e5f9100e"}
Message was sent while issue was closed.
Description was changed from ========== [Predator] Add maximum number argument to delta test. So the delta test can do something like "python run-delta-test.py -m 20" to check at most 20 crashes. BUG= ========== to ========== [Predator] Add maximum number argument to delta test. So the delta test can do something like "python run-delta-test.py -m 20" to check at most 20 crashes. BUG= Review-Url: https://codereview.chromium.org/2649523003 Committed: https://chromium.googlesource.com/infra/infra/+/54378e43b4346da07068052f91247... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/infra/infra/+/54378e43b4346da07068052f91247... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
