Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208143002:
[Findit] Filters for dashboard (Closed)
Description[Findit] Filters for dashboard
BUG=617808
Committed: https://chromium.googlesource.com/infra/infra/+/53e3ca2ffc8f48c3e854c0dcc2b8b3ad8cc4aff8
Patch Set 1 #
Total comments: 9
Patch Set 2 : lint, addressed comments, tests #
Total comments: 17
Patch Set 3 : addressed comments #Patch Set 4 : Merge branch 'master' of https://chromium.googlesource.com/infra/infra into dashboard-filter #
Messages
Total messages: 22 (7 generated)
caiw@google.com changed reviewers: + chanli@chromium.org, lijeffrey@chromium.org
I just have a couple of nits. I'll take another round of review when you upload tests. https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:16: Too many empty lines https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:26: master_flake_analyses = MasterFlakeAnalysis.query() Nit: change this argument name to things like master_flake_analyses_query?
Can you deploy a test version and share a link for what this modification will look like? https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:26: master_flake_analyses = MasterFlakeAnalysis.query() On 2016/08/03 21:32:50, chanli wrote: > Nit: change this argument name to things like master_flake_analyses_query? nit for the nit: master_flake_analysis_query https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:28: master_flake_analyses = master_flake_analyses.filter( nit: you can move the filtering part to a separate function to make unit testing more possible https://codereview.chromium.org/2208143002/diff/1/appengine/findit/templates/... File appengine/findit/templates/flake/dashboard.html (right): https://codereview.chromium.org/2208143002/diff/1/appengine/findit/templates/... appengine/findit/templates/flake/dashboard.html:9: nit: remove extra empty line, same with line 19
https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:16: On 2016/08/03 21:32:50, chanli wrote: > Too many empty lines Done. https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:26: master_flake_analyses = MasterFlakeAnalysis.query() On 2016/08/04 21:03:21, lijeffrey wrote: > On 2016/08/03 21:32:50, chanli wrote: > > Nit: change this argument name to things like master_flake_analyses_query? > > nit for the nit: master_flake_analysis_query Done. https://codereview.chromium.org/2208143002/diff/1/appengine/findit/handlers/f... appengine/findit/handlers/flake/filter_flake.py:28: master_flake_analyses = master_flake_analyses.filter( On 2016/08/04 21:03:21, lijeffrey wrote: > nit: you can move the filtering part to a separate function to make unit testing > more possible Done. https://codereview.chromium.org/2208143002/diff/1/appengine/findit/templates/... File appengine/findit/templates/flake/dashboard.html (right): https://codereview.chromium.org/2208143002/diff/1/appengine/findit/templates/... appengine/findit/templates/flake/dashboard.html:9: On 2016/08/04 21:03:21, lijeffrey wrote: > nit: remove extra empty line, same with line 19 Done.
I should mention the dashboard can be viewed at https://caiw4-dot-findit-for-me.appspot.com/waterfall/flake-dashboard
https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:44: master_flake_analysis_query = Filter( I suggested add query at the end because you had fetch later. Since you moved fetch to Filter(), you can remove this '_query' https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/test/filter_flake_test.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/test/filter_flake_test.py:88: print result Nit: remove this print
https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:44: master_flake_analysis_query = Filter( On 2016/08/05 23:29:41, chanli wrote: > I suggested add query at the end because you had fetch later. Since you moved > fetch to Filter(), you can remove this '_query' Done. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/test/filter_flake_test.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/test/filter_flake_test.py:88: print result On 2016/08/05 23:29:41, chanli wrote: > Nit: remove this print Done.
On 2016/08/08 20:41:49, caiw wrote: > https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... > File appengine/findit/handlers/flake/filter_flake.py (right): > > https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/flake/filter_flake.py:44: master_flake_analysis_query > = Filter( > On 2016/08/05 23:29:41, chanli wrote: > > I suggested add query at the end because you had fetch later. Since you moved > > fetch to Filter(), you can remove this '_query' > > Done. > > https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... > File appengine/findit/handlers/flake/test/filter_flake_test.py (right): > > https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/flake/test/filter_flake_test.py:88: print result > On 2016/08/05 23:29:41, chanli wrote: > > Nit: remove this print > > Done. lgtm
https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:12: def Filter(master_flake_analysis_query, master_name, builder_name, nit: Naming this Filter may cause confusion because of the built-in filter function. How about rename this something a bit more unique, maybe FilterMasterFlakeAnalysis() or something https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:12: def Filter(master_flake_analysis_query, master_name, builder_name, hm, to simplify the calls to this code, you should be able to make the filterable fields all default to none This way your code later can look like master_flake_analysis_query.Filter(master_flake_analysis_query, build_number=12345) instead of passing a bunch of Nones https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:48: for master_flake_analysis in master_flake_analysis_query: nit: 1 empty line before this for loop for readability https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:50: {'master_name': master_flake_analysis.master_name, nit: 'master_name' on a new line i.e. ...append({ 'master_name': ... 'builder_name': ... ... } https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:56: return { nit: add 1 empty line before return https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/test/filter_flake_test.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/test/filter_flake_test.py:81: self.assertTrue(len(result) == 1) you should be able to do self.assertEqual(len(result), 1). Same with the others.
https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:12: def Filter(master_flake_analysis_query, master_name, builder_name, On 2016/08/09 01:23:55, lijeffrey wrote: > hm, to simplify the calls to this code, you should be able to make the > filterable fields all default to none > > This way your code later can look like > > master_flake_analysis_query.Filter(master_flake_analysis_query, > build_number=12345) instead of passing a bunch of Nones I don't think this simplifies it right? If you want to exclude the Nones, you have to check in the handleget whether each field is None, and then call it with the non-none fields. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:12: def Filter(master_flake_analysis_query, master_name, builder_name, On 2016/08/09 01:23:55, lijeffrey wrote: > nit: Naming this Filter may cause confusion because of the built-in filter > function. How about rename this something a bit more unique, maybe > FilterMasterFlakeAnalysis() or something > Done. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:48: for master_flake_analysis in master_flake_analysis_query: On 2016/08/09 01:23:56, lijeffrey wrote: > nit: 1 empty line before this for loop for readability Done. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:50: {'master_name': master_flake_analysis.master_name, On 2016/08/09 01:23:55, lijeffrey wrote: > nit: 'master_name' on a new line > > i.e. > > ...append({ > 'master_name': ... > 'builder_name': ... > ... > } Done. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:56: return { On 2016/08/09 01:23:55, lijeffrey wrote: > nit: add 1 empty line before return Done. https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/test/filter_flake_test.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/test/filter_flake_test.py:81: self.assertTrue(len(result) == 1) On 2016/08/09 01:23:56, lijeffrey wrote: > you should be able to do self.assertEqual(len(result), 1). Same with the others. Done.
lgtm https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/flake/filter_flake.py (right): https://codereview.chromium.org/2208143002/diff/20001/appengine/findit/handle... appengine/findit/handlers/flake/filter_flake.py:12: def Filter(master_flake_analysis_query, master_name, builder_name, On 2016/08/09 19:37:02, caiw wrote: > On 2016/08/09 01:23:55, lijeffrey wrote: > > hm, to simplify the calls to this code, you should be able to make the > > filterable fields all default to none > > > > This way your code later can look like > > > > master_flake_analysis_query.Filter(master_flake_analysis_query, > > build_number=12345) instead of passing a bunch of Nones > > I don't think this simplifies it right? If you want to exclude the Nones, you > have to check in the handleget whether each field is None, and then call it with > the non-none fields. This is fine
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org Link to the patchset: https://codereview.chromium.org/2208143002/#ps40001 (title: "addressed 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: Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/308a6a153b171910) Infra Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/308a6a14b09ee510)
The CQ bit was checked by caiw@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, lijeffrey@chromium.org Link to the patchset: https://codereview.chromium.org/2208143002/#ps60001 (title: "Merge branch 'master' of https://chromium.googlesource.com/infra/infra into dashboard-filter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Findit] Filters for dashboard BUG=617808 ========== to ========== [Findit] Filters for dashboard BUG=617808 Committed: https://chromium.googlesource.com/infra/infra/+/53e3ca2ffc8f48c3e854c0dcc2b8b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/infra/infra/+/53e3ca2ffc8f48c3e854c0dcc2b8b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
