Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1591003002:
[Findit] Modify tryjob pipelines to trigger try jobs for test failure. (Closed)
Description[Findit] Modify tryjob pipelines to trigger try jobs for test failure.
Test failures are different from compile because there may be multiple steps and in each step there may be multiple tests, so the final result may contain multiple culprits for different tests.
Main task:
1. In each sub-pipeline, add logic to handle test failures.
2. Add a table to result page to display culprit for each step/test.
BUG=583806
Committed: https://chromium.googlesource.com/infra/infra/+/9ec3c4aed10f55037eb6f8a3dc623d537f6b33e9
Patch Set 1 #
Total comments: 12
Patch Set 2 : . #
Total comments: 12
Patch Set 3 : . #Patch Set 4 : Address comments. #Patch Set 5 : Address comments. #Patch Set 6 : Address comments. #Patch Set 7 : If different tests within the same step fail in different revisions, all revisions should be culpri… #
Total comments: 35
Patch Set 8 : Address comments. #Patch Set 9 : rebase #
Total comments: 14
Patch Set 10 : Address comments. #Patch Set 11 : Simplify the format for final try-job result for test failures. #
Total comments: 17
Patch Set 12 : Rebase and address conflicts. #
Total comments: 6
Patch Set 13 : Fix nits. #
Total comments: 8
Patch Set 14 : . #
Total comments: 10
Patch Set 15 : rebase #Patch Set 16 : . #
Total comments: 2
Patch Set 17 : . #Dependent Patchsets: Messages
Total messages: 36 (6 generated)
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org, qyearsley@chromium.org, stgao@chromium.org
Hi, This cl is about triggering try jobs for test failures. Please review it when you have time. Thank you.
https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:118: if result and len(result.get('result', [])) > 0: I think this can be reduced to just result.get('result', []) Sorry I added the len check and forgot to change it back by accident in my last CL, but functionally should be the same. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:131: if result and result.get('result'): it looks like this if statement can be moved outside the outer if/else https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:153: if failed_revisions: nit: maybe add if failed_revisions to _GetCulpritInfo to simplify the code here slightly https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/schedule_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/schedule_try_job_pipeline.py:31: if compile_targets: maybe move |if compile_targets| into the try_job_type == 'compile' branch and |if targeted_tests| into the else branch? I'd also add a comment for the else branch that all failures that are not compile are assumed to be test failures https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:53: if 'compile' in failed_steps: Just to double check, compile failures and test failures will never both show up in the same failed build? https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:74: test_last_pass < step_last_pass else step_last_pass) nit: add a few spaces to lign this up with the (
Since this is a relatively large change, is there more context or a bug link that could be added to the CL description? https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/model/... File appengine/findit/model/wf_try_job.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/model/... appengine/findit/model/wf_try_job.py:19: # A list of dict containing results and urls of each tryjob for test. Unrelated to this CL, but something that I notice: In some places we treat "tryjob" as one word, and in some places we treat it as two words (e.g. the name of this class, and on line 22 below, and in the name of this file). In the official documentation, it's treated as two words ("try job"). https://www.chromium.org/developers/testing/try-server-usage https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/run.sh (left): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/run.sh:59: echo "Code was synced successfully." Are the changes in run.sh related to the other changes in this CL, or could it be broken off into a separate CL? What's the purpose of the changes in run.sh, to make it possible to deploy when not synced to ToT? https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:34: self, master_name, builder_name, build_number, blame_list, try_job_type, Besides "compile", what other valid values are there for try_job_type? https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:36: """Identify the information for failed revisions. Identify -> Identifies This docstring means that this pipeline is used to fetch/collection information about failed revisions, right? From below, it looks like the main purpose is to identify culprits of failures based on try job results and provide information about those culprits right? https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:164: result['culprit'] = culprit_map This function is relatively long and nested, so it may be hard to understand later; might it be a good idea to extract some helper functions (e.g. one for the compile case and one for the non-compile case?) https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/try_job_util.py:138: need_new_try_job, last_pass, try_job_type, targeted_tests= ( space before =
https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/model/... File appengine/findit/model/wf_try_job.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/model/... appengine/findit/model/wf_try_job.py:19: # A list of dict containing results and urls of each tryjob for test. On 2016/01/17 03:43:12, qyearsley wrote: > Unrelated to this CL, but something that I notice: > > In some places we treat "tryjob" as one word, and in some places we treat it as > two words (e.g. the name of this class, and on line 22 below, and in the name of > this file). > > In the official documentation, it's treated as two words ("try job"). > https://www.chromium.org/developers/testing/try-server-usage Done. https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/util_s... File appengine/findit/util_scripts/run.sh (left): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/util_s... appengine/findit/util_scripts/run.sh:59: echo "Code was synced successfully." On 2016/01/17 03:43:12, qyearsley wrote: > Are the changes in run.sh related to the other changes in this CL, or could it > be broken off into a separate CL? > > What's the purpose of the changes in run.sh, to make it possible to deploy when > not synced to ToT? Sorry. There should be no change on run.sh... https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:34: self, master_name, builder_name, build_number, blame_list, try_job_type, On 2016/01/17 03:43:12, qyearsley wrote: > Besides "compile", what other valid values are there for try_job_type? 'test'. https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:36: """Identify the information for failed revisions. On 2016/01/17 03:43:12, qyearsley wrote: > Identify -> Identifies > This docstring means that this pipeline is used to fetch/collection information > about failed revisions, right? > > From below, it looks like the main purpose is to identify culprits of failures > based on try job results and provide information about those culprits right? Yes. From try job result we can get the failed revision. And in this pipeline we will get commit position and review url based on the revision. https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:164: result['culprit'] = culprit_map On 2016/01/17 03:43:12, qyearsley wrote: > This function is relatively long and nested, so it may be hard to understand > later; might it be a good idea to extract some helper functions (e.g. one for > the compile case and one for the non-compile case?) Done. https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/20001/appengine/findit/waterf... appengine/findit/waterfall/try_job_util.py:138: need_new_try_job, last_pass, try_job_type, targeted_tests= ( On 2016/01/17 03:43:12, qyearsley wrote: > space before = Done.
https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:118: if result and len(result.get('result', [])) > 0: On 2016/01/16 00:27:12, lijeffrey wrote: > I think this can be reduced to just result.get('result', []) Sorry I added the > len check and forgot to change it back by accident in my last CL, but > functionally should be the same. Done. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:131: if result and result.get('result'): On 2016/01/16 00:27:12, lijeffrey wrote: > it looks like this if statement can be moved outside the outer if/else Done. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:153: if failed_revisions: On 2016/01/16 00:27:12, lijeffrey wrote: > nit: maybe add if failed_revisions to _GetCulpritInfo to simplify the code here > slightly Done. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/schedule_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/schedule_try_job_pipeline.py:31: if compile_targets: On 2016/01/16 00:27:12, lijeffrey wrote: > maybe move |if compile_targets| into the try_job_type == 'compile' branch and > |if targeted_tests| into the else branch? > > I'd also add a comment for the else branch that all failures that are not > compile are assumed to be test failures Done. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:53: if 'compile' in failed_steps: On 2016/01/16 00:27:12, lijeffrey wrote: > Just to double check, compile failures and test failures will never both show up > in the same failed build? I think I've seen cases where compile fails with some other step, but I can't find it. But I think even if there is some other step failed, it should be compile related. So I guess it's safe to say we can only focus on compile failure if it happens. https://codereview.chromium.org/1591003002/diff/1/appengine/findit/waterfall/... appengine/findit/waterfall/try_job_util.py:74: test_last_pass < step_last_pass else step_last_pass) On 2016/01/16 00:27:12, lijeffrey wrote: > nit: add a few spaces to lign this up with the ( Done.
Hi, I just made another change to make sure if different tests within the same step fail in different revisions, all revisions should be culprits for the step. Please review it when you have time. Thanks.
Besides the comments inline with code, I feel that this CL is a little big and includes some change unrelated to the main purpose of this CL. After this CL, let's try to break big CLs into smaller ones. Take this CL as an example, it could be broken into three or four smaller ones: 1. gitignore. 2. rename "tryjob" to "try job". 3. kick off a try job run for test failures. 4. monitor and parse try job result. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... File appengine/findit/.gitignore (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh This comment might be misleading, as we need to track more than run.sh https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh As this is not relative to the main purpose of this CL, maybe create a separate CL for it? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh Does this work? See the conditions 3 and 4 below. (https://git-scm.com/docs/gitignore) To re-include files or directories when their parent directory is excluded, the following conditions must be met: 1. The rules to exclude a directory and re-include a subset back must be in the same .gitignore file. 2. The directory part in the re-include rules must be literal (i.e. no wildcards) 3. The rules to exclude the parent directory must not end with a trailing slash. 4. The rules to exclude the parent directory must have at least one slash. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/model... File appengine/findit/model/test/wf_try_job_test.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/model... appengine/findit/model/test/wf_try_job_test.py:14: try_job = WfTryJob.Create('m', 'b', 123) If you like, the rename here and in other files could go into a separate CL for such refactoring purpose. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:13: 'https://chromium.googlesource.com/chromium/src.git', HttpClient()) It seems in quite a few places in the code base, we use this url for quite a few times. We'd better reduce the occurrence of the same strings. How about filing a bug to track this and fixing it in a separate CL? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:33: # For test failures, the try job will run against every revision, Just as a side note: This is the case for now. But we'd like to improve the recipe findit/chromium/test and make it do the same as the findit/chromium/compile -- stop testing a test if the first failure is found. In this way, we could simplify the logic on Findit app side. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:39: for step, step_result in result['result'][revision].iteritems(): What if the revision is not tested yet due to infra failure, like exception during the recipe run? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:55: culprit_map[step]['tests'][failed_test] = {} Under the failed_test, is there a reason to have another level {'revision': revision}? Can it be just the string ``revision``? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:96: The format for final try-job result for test failures is: What do you think of adding a file recipe_result_format.md to explain the format of the recipe results and referring to the file here instead? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:203: result['culprit'] = culprits[failed_revisions[0]] Unrelated to this CL: In the else case, it means the try job can't identify the culprit for some reason, i.e. infra exception. Should we show a warming on the UI too? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:215: result_needs_update = ( nit: naming. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:44: if try_job_type == 'compile': use a CONSTANT instead? An enum might be better too. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:45: result_needs_update = try_job_result.compile_results the var name seems like a True/False flag, but it's actually not. How about naming it "result_to_update"? https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:57: if build.status == 'STARTED' and not already_set_started: unrelated to this CL: use enum or constant instead of hardcoded string. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:75: result_needs_update.append(result) Line #58-75 and #37-53 are very similar. How about extracting them into a separate function ``UpdateTryJobResult`` or the like? This could simplify unittest too, as we test the new function directly. To simplify unittest (especially setting up dummy data), we'd better go in this way later. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/schedule_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/schedule_try_job_pipeline.py:31: if targeted_tests: For test failures, we will always require the ``targeted_tests``, as we need to know which tests to run. Thus this ``if`` should be removed, and an assert should be added instead. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:61: targeted_tests = {} Alternative: extract this else branch into a helper function -- retrieve failed steps and tests.
https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... File appengine/findit/.gitignore (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh On 2016/01/26 00:51:40, stgao wrote: > As this is not relative to the main purpose of this CL, maybe create a separate > CL for it? Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh On 2016/01/26 00:51:40, stgao wrote: > Does this work? See the conditions 3 and 4 below. > > (https://git-scm.com/docs/gitignore) > > To re-include files or directories when their parent directory is excluded, the > following conditions must be met: > > 1. The rules to exclude a directory and re-include a subset back must be in the > same .gitignore file. > > 2. The directory part in the re-include rules must be literal (i.e. no > wildcards) > > 3. The rules to exclude the parent directory must not end with a trailing slash. > > 4. The rules to exclude the parent directory must have at least one slash. It works just fine for me. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh On 2016/01/26 00:51:40, stgao wrote: > This comment might be misleading, as we need to track more than run.sh Because I have some local scripts in util_scripts/ , I need to have these changes so git won't complain about those scripts. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/model... File appengine/findit/model/test/wf_try_job_test.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/model... appengine/findit/model/test/wf_try_job_test.py:14: try_job = WfTryJob.Create('m', 'b', 123) On 2016/01/26 00:51:40, stgao wrote: > If you like, the rename here and in other files could go into a separate CL for > such refactoring purpose. Got it. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:13: 'https://chromium.googlesource.com/chromium/src.git', HttpClient()) On 2016/01/26 00:51:40, stgao wrote: > It seems in quite a few places in the code base, we use this url for quite a few > times. > > We'd better reduce the occurrence of the same strings. > How about filing a bug to track this and fixing it in a separate CL? Acknowledged. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:33: # For test failures, the try job will run against every revision, On 2016/01/26 00:51:40, stgao wrote: > Just as a side note: This is the case for now. But we'd like to improve the > recipe findit/chromium/test and make it do the same as the > findit/chromium/compile -- stop testing a test if the first failure is found. In > this way, we could simplify the logic on Findit app side. Acknowledged. I'll keep it in mind and make the change when the recipe is ready. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:39: for step, step_result in result['result'][revision].iteritems(): On 2016/01/26 00:51:40, stgao wrote: > What if the revision is not tested yet due to infra failure, like exception > during the recipe run? If there were any infra failure or something else, data['result'] should be 'FAILURE' and there would be data['failure_reason'] as well, right? I'll modify buildbucket_client a little to include those info. Then I'll add a check on data['result'] == 'SUCCESS' and I'll handle other cases in a separate CL. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:55: culprit_map[step]['tests'][failed_test] = {} On 2016/01/26 00:51:40, stgao wrote: > Under the failed_test, is there a reason to have another level {'revision': > revision}? Can it be just the string ``revision``? Just like the example below, we will record the culprit info for each test as well: 'tests': { 'a_test1': { 'revision': 'rev1', 'commit_position': '1', 'review_url': 'url_1' }, 'a_test2': { 'revision': 'rev2', 'commit_position': '2', 'review_url': 'url_2' } https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:96: The format for final try-job result for test failures is: On 2016/01/26 00:51:40, stgao wrote: > What do you think of adding a file recipe_result_format.md to explain the format > of the recipe results and referring to the file here instead? Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:203: result['culprit'] = culprits[failed_revisions[0]] On 2016/01/26 00:51:40, stgao wrote: > Unrelated to this CL: In the else case, it means the try job can't identify the > culprit for some reason, i.e. infra exception. > Should we show a warming on the UI too? Acknowledged. I'll file a bug for this as well. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:215: result_needs_update = ( On 2016/01/26 00:51:40, stgao wrote: > nit: naming. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:44: if try_job_type == 'compile': On 2016/01/26 00:51:40, stgao wrote: > use a CONSTANT instead? An enum might be better too. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:45: result_needs_update = try_job_result.compile_results On 2016/01/26 00:51:40, stgao wrote: > the var name seems like a True/False flag, but it's actually not. > How about naming it "result_to_update"? Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:57: if build.status == 'STARTED' and not already_set_started: On 2016/01/26 00:51:40, stgao wrote: > unrelated to this CL: use enum or constant instead of hardcoded string. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:75: result_needs_update.append(result) On 2016/01/26 00:51:40, stgao wrote: > Line #58-75 and #37-53 are very similar. How about extracting them into a > separate function ``UpdateTryJobResult`` or the like? > > This could simplify unittest too, as we test the new function directly. > To simplify unittest (especially setting up dummy data), we'd better go in this > way later. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/schedule_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/schedule_try_job_pipeline.py:31: if targeted_tests: On 2016/01/26 00:51:41, stgao wrote: > For test failures, we will always require the ``targeted_tests``, as we need to > know which tests to run. > > Thus this ``if`` should be removed, and an assert should be added instead. Done. https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:61: targeted_tests = {} On 2016/01/26 00:51:41, stgao wrote: > Alternative: extract this else branch into a helper function -- retrieve failed > steps and tests. Done.
https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... File appengine/findit/.gitignore (right): https://codereview.chromium.org/1591003002/diff/120001/appengine/findit/.giti... appengine/findit/.gitignore:7: # but do track run.sh On 2016/01/27 18:49:55, chanli wrote: > On 2016/01/26 00:51:40, stgao wrote: > > This comment might be misleading, as we need to track more than run.sh > > Because I have some local scripts in util_scripts/ , I need to have these > changes so git won't complain about those scripts. Moved to another CL.
Some more comments. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:47: try_job_result.status = wf_analysis_status.ANALYZING What if status == 'COMPLETED'? https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:66: elif build.status == TryJobStatus.completed: Isn't the enum available in common.buildbucket_client.BuildbucketBuild already? https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py:112: targeted_tests = ['browser_test'] Shouldn't we specify the testcase list too? https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:7: type_compile = 'compile' IIRC, constant usually use upper-case instead of lower-case for var names. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:8: type_test = 'test' It seems the 'type' in the var name is a bit redundant, as the enum class name already indicates that. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:11: class TryJobStatus(): It seems duplicating common.buildbucket_client.BuildbucketBuild https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:8: 'try_job_id': '1', This isn't included in the raw try job result, right? Maybe we'd better make it more clear whether this is the recipe result from the try job, or the result we save in datastore for a try job. I would suggest adding the recipe result format here too, because the recipes are in a different code repo and the returned format there is not obvious.
https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:47: try_job_result.status = wf_analysis_status.ANALYZING On 2016/01/28 19:33:51, stgao wrote: > What if status == 'COMPLETED'? I will change the try_job_result.status to ANALYZED when the identify_try_job_pipeline finishes, before that it will be ANALYZING. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:66: elif build.status == TryJobStatus.completed: On 2016/01/28 19:33:52, stgao wrote: > Isn't the enum available in common.buildbucket_client.BuildbucketBuild already? Done. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py:112: targeted_tests = ['browser_test'] On 2016/01/28 19:33:52, stgao wrote: > Shouldn't we specify the testcase list too? This test is just to test starting a tryjob for test with right parameters. I don't think it's necessary to make the targeted_test multiple layers, it's not so relevant to this test. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:7: type_compile = 'compile' On 2016/01/28 19:33:52, stgao wrote: > IIRC, constant usually use upper-case instead of lower-case for var names. Done. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:8: type_test = 'test' On 2016/01/28 19:33:52, stgao wrote: > It seems the 'type' in the var name is a bit redundant, as the enum class name > already indicates that. Done. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:11: class TryJobStatus(): On 2016/01/28 19:33:52, stgao wrote: > It seems duplicating common.buildbucket_client.BuildbucketBuild Done. https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/160001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:8: 'try_job_id': '1', On 2016/01/28 19:33:52, stgao wrote: > This isn't included in the raw try job result, right? > > Maybe we'd better make it more clear whether this is the recipe result from the > try job, or the result we save in datastore for a try job. > > I would suggest adding the recipe result format here too, because the recipes > are in a different code repo and the returned format there is not obvious. This is the final result saved in WfTryJob.compile_results or WfTryJob.test_results. I'll add the recipe result format
Hi, I made some change to simplify the format for final try-job result for test failures. Please take a look when you have time. Thanks.
https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:25: if change_log: If the request to Gitiles fails for some reason, do we still want to return the git hash only? If so, maybe leave a TODO or file a bug. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:72: test_culprit.update(culprits[test_revision]) If line #25 is false, this will cause an exception. ``culprits.get(test_revision, {'revision': test_revision})`` could help, I guess. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:285: expected_test_result = { style nit: too many space. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py:112: targeted_tests = ['browser_test'] But this parameter doesn't look right to me. The recipe expects a dict mapping from test step name to failed tests. This could cause confusion later. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. 2016 Maybe check other new files too, if any. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:27: # The format for recipe result for compile failures is: typo? https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:69: targeted_tests.pop(failure_name) Alternative: save the returned value first and add it to ``targeted_tests`` if needed here.
The CL is almost in good shape now :)
https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:75: def run( you may want to do a rebase, a lot of code has changed and was refactored from https://codereview.chromium.org/1622813003/
Hi, I finally finished the merge... Happy Friday : ) https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:25: if change_log: On 2016/01/29 18:55:01, stgao wrote: > If the request to Gitiles fails for some reason, do we still want to return the > git hash only? > > If so, maybe leave a TODO or file a bug. Yes i think we should return the hash anyway. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:72: test_culprit.update(culprits[test_revision]) On 2016/01/29 18:55:02, stgao wrote: > If line #25 is false, this will cause an exception. > ``culprits.get(test_revision, {'revision': test_revision})`` could help, I > guess. Made the change to include revision, this is fine now. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:75: def run( On 2016/01/29 19:49:46, lijeffrey wrote: > you may want to do a rebase, a lot of code has changed and was refactored from > https://codereview.chromium.org/1622813003/ Done. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:285: expected_test_result = { On 2016/01/29 18:55:02, stgao wrote: > style nit: too many space. Done. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/test/schedule_try_job_pipeline_test.py:112: targeted_tests = ['browser_test'] On 2016/01/29 18:55:02, stgao wrote: > But this parameter doesn't look right to me. The recipe expects a dict mapping > from test step name to failed tests. > > This could cause confusion later. Done. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/29 18:55:02, stgao wrote: > 2016 > > Maybe check other new files too, if any. Done. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:27: # The format for recipe result for compile failures is: On 2016/01/29 18:55:02, stgao wrote: > typo? Done. https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:69: targeted_tests.pop(failure_name) On 2016/01/29 18:55:02, stgao wrote: > Alternative: save the returned value first and add it to ``targeted_tests`` if > needed here. Done.
lgtm with nits. https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/test/try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/test/try_job_pipeline_test.py:125: print json.dumps(try_job.compile_results, indent=4) This should be removed before commit. https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. nit: The file name seems not matching the class name within this module.
https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:3: ['rev1', 'passed'], This now has to be updated after https://codereview.chromium.org/1615963005/
https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/test/try_job_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/test/try_job_pipeline_test.py:125: print json.dumps(try_job.compile_results, indent=4) On 2016/02/01 19:09:01, stgao wrote: > This should be removed before commit. Done. https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/try_job_enums.py (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/try_job_enums.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2016/02/01 19:09:01, stgao wrote: > nit: The file name seems not matching the class name within this module. Done. https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/220001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:3: ['rev1', 'passed'], On 2016/02/01 19:22:01, stgao wrote: > This now has to be updated after https://codereview.chromium.org/1615963005/ Done.
lgtm with nits https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/200001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:28: 'result': result_content, nit: for compile, this has been renamed to 'report', should we do the same for test failures too? https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:158: compile_result = { Since the compile recipe has now changed, the format 'report': [...] is no longer a possible testcase, so this should be updated to 'report': { 'result': { 'rev1': 'passed', 'rev2': 'passed' } } https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:59: failure_targeted_tests, failure_need_try_job, failure_last_pass =( nit: = ( https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:89: if 'compile' in failed_steps: just an idea, maybe we should make this a separate function to determine whether we want to trigger a compile or test try job, maybe something like def _GetTryJobTypeForFailedSteps(failed_steps): if 'compile' in failed_steps: return TryJobType.COMPILE return TryJobType.TEST up to you though https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:97: targeted_tests, need_new_try_job, last_pass =( nit: = (
https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py (right): https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:158: compile_result = { On 2016/02/02 00:24:31, lijeffrey wrote: > Since the compile recipe has now changed, the format 'report': [...] is no > longer a possible testcase, so this should be updated to > > 'report': { > 'result': { > 'rev1': 'passed', > 'rev2': 'passed' > } > } Done. https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... File appengine/findit/waterfall/try_job_util.py (right): https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:59: failure_targeted_tests, failure_need_try_job, failure_last_pass =( On 2016/02/02 00:24:31, lijeffrey wrote: > nit: = ( Done. https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:89: if 'compile' in failed_steps: On 2016/02/02 00:24:31, lijeffrey wrote: > just an idea, maybe we should make this a separate function to determine whether > we want to trigger a compile or test try job, maybe something like > > def _GetTryJobTypeForFailedSteps(failed_steps): > if 'compile' in failed_steps: > return TryJobType.COMPILE > return TryJobType.TEST > > up to you though I think I'll leave it as is since there is more than setting try_job_type for different cases. https://codereview.chromium.org/1591003002/diff/240001/appengine/findit/water... appengine/findit/waterfall/try_job_util.py:97: targeted_tests, need_new_try_job, last_pass =( On 2016/02/02 00:24:31, lijeffrey wrote: > nit: = ( Done.
Is there a BUG= for this CL? https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:132: if step_result['valid'] and step_result['status'] == 'failed': Optional change if you want to use one less level of indentation: if not step_result['valid'] or step_result['status'] != 'failed': continue Either way is probably equally good. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:209: return result.get('culprit', None) if result else None Equivalent to `result.get('culprit') if result else None`, since the default default for dict.get() is None. But this is good as is too, since it is explicit. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:57: timeout_hours = 5 # Timeout after 5 hours. This comment is redundant and can be removed. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:10: It's very nice to have a document describing the format like this, although I think it can be improved. When this md file is converted to HTML for viewing, the examples will only be displayed in "code block" style if they are indented one more level: The format for recipe result for compile failures is: { 'report': { 'result': { 'rev1': 'passed', 'rev2': 'failed' } } } Also, # Indicates a top-level header. You can see roughly how this document will look when converted to HTML by using tools like http://dillinger.io/. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/try_job_type.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/try_job_type.py:6: class TryJobType(): I think it's recommend to inherit from object, i.e. `class TryJobType(object)`. https://wiki.python.org/moin/NewClassVsClassicClass Although practically speaking, I'm sure it doesn't matter a lot for this particular case where we're basically just making a simple enum.
Description was changed from ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. BUG= ========== to ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. BUG=583806 ==========
On 2016/02/03 18:55:14, qyearsley wrote: > Is there a BUG= for this CL? > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:132: if > step_result['valid'] and step_result['status'] == 'failed': > Optional change if you want to use one less level of indentation: > > if not step_result['valid'] or step_result['status'] != 'failed': > continue > > Either way is probably equally good. > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:209: return > result.get('culprit', None) if result else None > Equivalent to `result.get('culprit') if result else None`, since the default > default for dict.get() is None. But this is good as is too, since it is > explicit. > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > appengine/findit/waterfall/monitor_try_job_pipeline.py:57: timeout_hours = 5 # > Timeout after 5 hours. > This comment is redundant and can be removed. > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > File appengine/findit/waterfall/try_job_result_format.md (right): > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > appengine/findit/waterfall/try_job_result_format.md:10: > It's very nice to have a document describing the format like this, although I > think it can be improved. > > When this md file is converted to HTML for viewing, the examples will only be > displayed in "code block" style if they are indented one more level: > > > The format for recipe result for compile failures is: > > { > 'report': { > 'result': { > 'rev1': 'passed', > 'rev2': 'failed' > } > } > } > > > Also, # Indicates a top-level header. You can see roughly how this document will > look when converted to HTML by using tools like http://dillinger.io/. > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > File appengine/findit/waterfall/try_job_type.py (right): > > https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... > appengine/findit/waterfall/try_job_type.py:6: class TryJobType(): > I think it's recommend to inherit from object, i.e. `class TryJobType(object)`. > > https://wiki.python.org/moin/NewClassVsClassicClass > > Although practically speaking, I'm sure it doesn't matter a lot for this > particular case where we're basically just making a simple enum. There was a bug for compile but not for test. I created a new one for this change.
https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:132: if step_result['valid'] and step_result['status'] == 'failed': On 2016/02/03 18:55:14, qyearsley wrote: > Optional change if you want to use one less level of indentation: > > if not step_result['valid'] or step_result['status'] != 'failed': > continue > > Either way is probably equally good. Done. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:209: return result.get('culprit', None) if result else None On 2016/02/03 18:55:14, qyearsley wrote: > Equivalent to `result.get('culprit') if result else None`, since the default > default for dict.get() is None. But this is good as is too, since it is > explicit. Done. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/monitor_try_job_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/monitor_try_job_pipeline.py:57: timeout_hours = 5 # Timeout after 5 hours. On 2016/02/03 18:55:14, qyearsley wrote: > This comment is redundant and can be removed. Done. https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/try_job_result_format.md (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/try_job_result_format.md:10: On 2016/02/03 18:55:14, qyearsley wrote: > It's very nice to have a document describing the format like this, although I > think it can be improved. > > When this md file is converted to HTML for viewing, the examples will only be > displayed in "code block" style if they are indented one more level: > > > The format for recipe result for compile failures is: > > { > 'report': { > 'result': { > 'rev1': 'passed', > 'rev2': 'failed' > } > } > } > > > Also, # Indicates a top-level header. You can see roughly how this document will > look when converted to HTML by using tools like http://dillinger.io/. Yes. It does look much better now. Thank you : ) https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... File appengine/findit/waterfall/try_job_type.py (right): https://codereview.chromium.org/1591003002/diff/260001/appengine/findit/water... appengine/findit/waterfall/try_job_type.py:6: class TryJobType(): On 2016/02/03 18:55:14, qyearsley wrote: > I think it's recommend to inherit from object, i.e. `class TryJobType(object)`. > > https://wiki.python.org/moin/NewClassVsClassicClass > > Although practically speaking, I'm sure it doesn't matter a lot for this > particular case where we're basically just making a simple enum. Done.
lgtm
lgtm with a nit. https://codereview.chromium.org/1591003002/diff/300001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/300001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:135: continue nit: an empty line here will improve readability.
https://codereview.chromium.org/1591003002/diff/300001/appengine/findit/water... File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right): https://codereview.chromium.org/1591003002/diff/300001/appengine/findit/water... appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:135: continue On 2016/02/04 00:19:47, stgao wrote: > nit: an empty line here will improve readability. Done.
The CQ bit was checked by chanli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lijeffrey@chromium.org, stgao@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/1591003002/#ps320001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591003002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591003002/320001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. BUG=583806 ========== to ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. BUG=583806 Committed: https://chromium.googlesource.com/infra/infra/+/9ec3c4aed10f55037eb6f8a3dc623... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/infra/infra/+/9ec3c4aed10f55037eb6f8a3dc623...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. BUG=583806 Committed: https://chromium.googlesource.com/infra/infra/+/9ec3c4aed10f55037eb6f8a3dc623... ========== to ========== [Findit] Modify tryjob pipelines to trigger try jobs for test failure. Test failures are different from compile because there may be multiple steps and in each step there may be multiple tests, so the final result may contain multiple culprits for different tests. Main task: 1. In each sub-pipeline, add logic to handle test failures. 2. Add a table to result page to display culprit for each step/test. BUG=583806 Committed: https://chromium.googlesource.com/infra/infra/+/9ec3c4aed10f55037eb6f8a3dc623... ========== |
