Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042043004:
Added skeleton code for the Detection of regression range for flaky (Closed)
Description[Findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters.
BUG=617808
Committed: https://chromium.googlesource.com/infra/infra/+/c1b22a789cbe9d000290bbdc9455a6b98778a8f6
Patch Set 1 #Patch Set 2 : initial #
Total comments: 27
Patch Set 3 : Skeleton Code Regression Range #
Total comments: 9
Patch Set 4 : [findit] fixed most recent comments #
Total comments: 2
Patch Set 5 : [Findit] fixed previous comments #
Total comments: 2
Messages
Total messages: 25 (9 generated)
caiw@google.com changed reviewers: + stgao@chromium.org
stgao@chromium.org changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:9: class CheckFlaky(BaseHandler): style nit: one more line above. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:18: testcase_list = self.request.get('testcase_list').strip() To make it simpler, how about starting with a single testcase? If we do multiple testcases, the Swarming reruns will be not that straightforward. Because different flaky tests might have different regression ranges. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( As we only need the task id for the specific test step (implied by the test_target_name), we could filter by master_name, builder_name, build_number, and step_name or name(test_target_name). https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( This will send one or more http request to the Swarming server, and it might take a while. In order to avoid the 1-minute deadline for the frontend http request, it seems better to move this to the App Engine task or App Engine pipeline which will run on the module waterfall-backend. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.py File appengine/findit/main.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.p... appengine/findit/main.py:28: from handlers.flake import check_flaky nit: check_flake? https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/templa... File appengine/findit/templates/flake.html (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/templa... appengine/findit/templates/flake.html:14: <input type="submit" value="Check Flaky"> nit: "Find regression range"? Same for title and header above.
https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:13: #get input parameters Style nit: comments should be in the form # Get input parameters. <-- space after #, capital first letter, and end with some punctuation. Before submitting your code for review you can run gpylint <path to .py file> And it'll find most style nits for you https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:19: print(master_name, builder_name, build_number, This print looks like it's for debugging. Be sure to remove it before committing the code. If it's actual logging you want you can use the logging module and look at some of the other code in Findit for how to use it https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:27: # TODO: trigger swarming reruns Nit: for TODOs, if you plan to do them yourself, you should put your ldap after or a bug number. i.e. # TODO(caiw): Trigger swarming reruns. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.py File appengine/findit/main.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.p... appengine/findit/main.py:61: ('/waterfall/check-flaky', check_flaky.CheckFlaky), nit: I think these are alphabetized, so put this one after check-duplicate-failures
https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:22: # get task id Style nit like ln 13
Hi, I think I addressed the issues with the previous commit and added more of a framework. Thanks, Will
Posted a few more comments. Besides, I'd usually we prefix the first line of CL description with "[Findit]". This makes it easier to tell which project a CL is for. And we could refer to the bug http://crbug.com/617808 by adding below as one line in the CL description. BUG=617808 https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:22: # TODO(caiw): Get status of master_analysis from database nit: end with a dot like this. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:32: # TODO(caiw): Trigger pipeline Same here. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:36: def HandlePost(self): # pragma: no cover This seems won't be used, thus we could delete it for now.
Description was changed from ========== Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. ========== to ========== [findit] BUG=617808 Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. ==========
Description was changed from ========== [findit] BUG=617808 Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. ========== to ========== [findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 ==========
https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/app.yaml File appengine/findit/app.yaml (right): https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/app.ya... appengine/findit/app.yaml:29: - url: /flake How about /waterfall/flake? And move it to waterfall-frontend.yaml instead?
[findit] fixed most recent comments https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:9: class CheckFlaky(BaseHandler): On 2016/06/08 17:24:12, stgao wrote: > style nit: one more line above. Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:13: #get input parameters On 2016/06/08 18:05:56, lijeffrey wrote: > Style nit: > > comments should be in the form > > # Get input parameters. <-- space after #, capital first letter, and end with > some punctuation. > > Before submitting your code for review you can run > > gpylint <path to .py file> > > And it'll find most style nits for you Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:18: testcase_list = self.request.get('testcase_list').strip() On 2016/06/08 17:24:12, stgao wrote: > To make it simpler, how about starting with a single testcase? > > If we do multiple testcases, the Swarming reruns will be not that > straightforward. Because different flaky tests might have different regression > ranges. Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:19: print(master_name, builder_name, build_number, On 2016/06/08 18:05:56, lijeffrey wrote: > This print looks like it's for debugging. Be sure to remove it before committing > the code. > > If it's actual logging you want you can use the logging module and look at some > of the other code in Findit for how to use it Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:19: print(master_name, builder_name, build_number, On 2016/06/08 18:05:56, lijeffrey wrote: > This print looks like it's for debugging. Be sure to remove it before committing > the code. > > If it's actual logging you want you can use the logging module and look at some > of the other code in Findit for how to use it Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:22: # get task id On 2016/06/08 20:46:00, chanli wrote: > Style nit like ln 13 Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:22: # get task id On 2016/06/08 20:46:00, chanli wrote: > Style nit like ln 13 Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( On 2016/06/08 17:24:12, stgao wrote: > As we only need the task id for the specific test step (implied by the > test_target_name), we could filter by master_name, builder_name, build_number, > and step_name or name(test_target_name). Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( On 2016/06/08 17:24:12, stgao wrote: > As we only need the task id for the specific test step (implied by the > test_target_name), we could filter by master_name, builder_name, build_number, > and step_name or name(test_target_name). Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( On 2016/06/08 17:24:12, stgao wrote: > This will send one or more http request to the Swarming server, and it might > take a while. In order to avoid the 1-minute deadline for the frontend http > request, it seems better to move this to the App Engine task or App Engine > pipeline which will run on the module waterfall-backend. Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:27: # TODO: trigger swarming reruns On 2016/06/08 18:05:57, lijeffrey wrote: > Nit: for TODOs, if you plan to do them yourself, you should put your ldap after > or a bug number. > > i.e. > > # TODO(caiw): Trigger swarming reruns. Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.py File appengine/findit/main.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.p... appengine/findit/main.py:28: from handlers.flake import check_flaky On 2016/06/08 17:24:12, stgao wrote: > nit: check_flake? Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/main.p... appengine/findit/main.py:61: ('/waterfall/check-flaky', check_flaky.CheckFlaky), On 2016/06/08 18:05:57, lijeffrey wrote: > nit: I think these are alphabetized, so put this one after > check-duplicate-failures Acknowledged. https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/templa... File appengine/findit/templates/flake.html (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/templa... appengine/findit/templates/flake.html:14: <input type="submit" value="Check Flaky"> On 2016/06/08 17:24:12, stgao wrote: > nit: "Find regression range"? > > Same for title and header above. Acknowledged. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:22: # TODO(caiw): Get status of master_analysis from database On 2016/06/13 22:27:11, stgao wrote: > nit: end with a dot like this. Acknowledged. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:32: # TODO(caiw): Trigger pipeline On 2016/06/13 22:27:12, stgao wrote: > Same here. Acknowledged. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:36: def HandlePost(self): # pragma: no cover On 2016/06/13 22:27:12, stgao wrote: > This seems won't be used, thus we could delete it for now. Done.
Fixed most recent comments (sorry I think I published last time before actually uploading the new cl) https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/app.yaml File appengine/findit/app.yaml (right): https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/app.ya... appengine/findit/app.yaml:29: - url: /flake On 2016/06/13 22:29:43, stgao wrote: > How about /waterfall/flake? > And move it to waterfall-frontend.yaml instead? Done. https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/40001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:22: # TODO(caiw): Get status of master_analysis from database On 2016/06/13 22:27:11, stgao wrote: > nit: end with a dot like this. Done.
https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:18: testcase_list = self.request.get('testcase_list').strip() On 2016/06/13 22:43:56, caiw wrote: > On 2016/06/08 17:24:12, stgao wrote: > > To make it simpler, how about starting with a single testcase? > > > > If we do multiple testcases, the Swarming reruns will be not that > > straightforward. Because different flaky tests might have different regression > > ranges. > > Acknowledged. It seems that this comment was not addressed in the latest patch. What's your thought? https://codereview.chromium.org/2042043004/diff/60001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/60001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:5: # pylint: disable=W0612 nit: move this next to the unused variables.
Description was changed from ========== [findit] BUG=617808 Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. ========== to ========== [findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 ==========
Description was changed from ========== [findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 ========== to ========== [Findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 ==========
lgtm
You may also want to add a skeleton test file to handlers/test/check_flake_test.py too if code coverage isn't 100% with this new module but otherwise lgtm https://codereview.chromium.org/2042043004/diff/80001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/80001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:16: master_name = self.request.get('master_name').strip() will these cause exceptions if not specified in the request? i.e. request.get('master_name') evaluates to None, then calling .strip(). Maybe add some default values, like request.get('master_name', '') to avoid this?
https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flaky.py (right): https://codereview.chromium.org/2042043004/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/check_flaky.py:23: tasks = swarming_util.ListSwarmingTasksDataByTags( On 2016/06/08 17:24:12, stgao wrote: > As we only need the task id for the specific test step (implied by the > test_target_name), we could filter by master_name, builder_name, build_number, > and step_name or name(test_target_name). Acknowledged. https://codereview.chromium.org/2042043004/diff/60001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/60001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:5: # pylint: disable=W0612 On 2016/06/13 22:56:12, stgao wrote: > nit: move this next to the unused variables. Done. https://codereview.chromium.org/2042043004/diff/80001/appengine/findit/handle... File appengine/findit/handlers/flake/check_flake.py (right): https://codereview.chromium.org/2042043004/diff/80001/appengine/findit/handle... appengine/findit/handlers/flake/check_flake.py:16: master_name = self.request.get('master_name').strip() On 2016/06/14 00:05:58, lijeffrey wrote: > will these cause exceptions if not specified in the request? > > > i.e. request.get('master_name') evaluates to None, then calling .strip(). > > Maybe add some default values, like request.get('master_name', '') to avoid > this? Will do
The CQ bit was checked by caiw@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042043004/80001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 ========== to ========== [Findit] Added skeleton code for the Detection of regression range for flaky tests project. Has a simple UI page to take input parameters. BUG=617808 Committed: https://chromium.googlesource.com/infra/infra/+/c1b22a789cbe9d000290bbdc9455a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/infra/infra/+/c1b22a789cbe9d000290bbdc9455a...
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
