Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1949763002:
[Findit] Adding try job dashboard (Closed)
Description[Findit] Adding try job dashboard
For sample dashboard, refer to:
https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/try-job-dashboard
BUG=608896
Committed: https://chromium.googlesource.com/infra/infra/+/ac711279b23ea8f985d2a1c2fe84a30fb5ba78f8
Patch Set 1 #Patch Set 2 : Fixing bug where fields are missing from data #
Total comments: 16
Patch Set 3 : Addressing comments and adding unit tests #
Total comments: 8
Patch Set 4 : Addressing nits #
Messages
Total messages: 14 (5 generated)
Description was changed from ========== [Findit] Adding try job dashboard BUG= ========== to ========== [Findit] Adding try job dashboard For sample dashboard, refer to: https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/try-job-dashboard BUG=608896 ==========
lijeffrey@chromium.org changed reviewers: + chanli@chromium.org, stgao@chromium.org
This is an initial review for the try job dashboard. Currently I am working on unit tests to get code coverage which I'll upload in the next patch. PTAL and refer to the link to see how the dashboard looks and works when you have the chance
https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/try_job_dashboard_test.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/try_job_dashboard_test.py:10: from testing_utils import testing After recent change, I think you clould import common.findit_test_case or waterfall.wf_testcase https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... File appengine/findit/templates/try_job_dashboard.html (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:33: return false; // TODO(lijeffrey): This is a hack to get the url to update. I'm not sure if that'll work but have you tried window.location.reload()?
https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/try_job_dashboard.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:47: wf_analysis_query = WfTryJobData.query( naming nit: wf_analysis_query. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:86: try_job_display_data['end_time'] = try_job_data.end_time time format. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:107: def HandlePost(self): # pragma: no cover We could remove this one. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.py File appengine/findit/main.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.p... appengine/findit/main.py:73: ('/waterfall/try-job-dashboard', try_job_dashboard.TryJobDashboard), nit: ordering. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... File appengine/findit/templates/try_job_dashboard.html (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:31: newUrl = createUrl(); Should we pass the ``parameters`` to function createUrl instead of a global variable above? https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:65: {% for try_job_display_data in try_jobs_in_progress %} Maybe show "No in-progress try-jobs" if the list is empty?
Comments addressed, ptal. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/test/try_job_dashboard_test.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/test/try_job_dashboard_test.py:10: from testing_utils import testing On 2016/05/03 23:58:31, chanli wrote: > After recent change, I think you clould import common.findit_test_case or > waterfall.wf_testcase Right now it seems that's not necessary since the dashboard doesn't rely on any config settings https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... File appengine/findit/handlers/try_job_dashboard.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:47: wf_analysis_query = WfTryJobData.query( On 2016/05/04 00:00:53, stgao wrote: > naming nit: wf_analysis_query. Oops, done. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:86: try_job_display_data['end_time'] = try_job_data.end_time On 2016/05/04 00:00:53, stgao wrote: > time format. Done. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:107: def HandlePost(self): # pragma: no cover On 2016/05/04 00:00:53, stgao wrote: > We could remove this one. Done. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.py File appengine/findit/main.py (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.p... appengine/findit/main.py:73: ('/waterfall/try-job-dashboard', try_job_dashboard.TryJobDashboard), On 2016/05/04 00:00:53, stgao wrote: > nit: ordering. Done. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... File appengine/findit/templates/try_job_dashboard.html (right): https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:31: newUrl = createUrl(); On 2016/05/04 00:00:53, stgao wrote: > Should we pass the ``parameters`` to function createUrl instead of a global > variable above? Done. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:33: return false; // TODO(lijeffrey): This is a hack to get the url to update. On 2016/05/03 23:58:31, chanli wrote: > I'm not sure if that'll work but have you tried window.location.reload()? reload() doesn't seem to work, but http://stackoverflow.com/questions/3678515/very-strange-calling-window-locati... seems that return false is standard for what we want? In the case of config.html, the previous/next buttons are actual buttons while here it's a submit form, though I haven't tested using a regular button here vs a "submit" button in config. https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:65: {% for try_job_display_data in try_jobs_in_progress %} On 2016/05/04 00:00:53, stgao wrote: > Maybe show "No in-progress try-jobs" if the list is empty? Done.
On 2016/05/04 20:20:17, lijeffrey wrote: > Comments addressed, ptal. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > File appengine/findit/handlers/test/try_job_dashboard_test.py (right): > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/test/try_job_dashboard_test.py:10: from testing_utils > import testing > On 2016/05/03 23:58:31, chanli wrote: > > After recent change, I think you clould import common.findit_test_case or > > waterfall.wf_testcase > > Right now it seems that's not necessary since the dashboard doesn't rely on any > config settings > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > File appengine/findit/handlers/try_job_dashboard.py (right): > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/try_job_dashboard.py:47: wf_analysis_query = > WfTryJobData.query( > On 2016/05/04 00:00:53, stgao wrote: > > naming nit: wf_analysis_query. > > Oops, done. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/try_job_dashboard.py:86: > try_job_display_data['end_time'] = try_job_data.end_time > On 2016/05/04 00:00:53, stgao wrote: > > time format. > > Done. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/handle... > appengine/findit/handlers/try_job_dashboard.py:107: def HandlePost(self): # > pragma: no cover > On 2016/05/04 00:00:53, stgao wrote: > > We could remove this one. > > Done. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.py > File appengine/findit/main.py (right): > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/main.p... > appengine/findit/main.py:73: ('/waterfall/try-job-dashboard', > try_job_dashboard.TryJobDashboard), > On 2016/05/04 00:00:53, stgao wrote: > > nit: ordering. > > Done. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... > File appengine/findit/templates/try_job_dashboard.html (right): > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... > appengine/findit/templates/try_job_dashboard.html:31: newUrl = createUrl(); > On 2016/05/04 00:00:53, stgao wrote: > > Should we pass the ``parameters`` to function createUrl instead of a global > > variable above? > > Done. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... > appengine/findit/templates/try_job_dashboard.html:33: return false; // > TODO(lijeffrey): This is a hack to get the url to update. > On 2016/05/03 23:58:31, chanli wrote: > > I'm not sure if that'll work but have you tried window.location.reload()? > > reload() doesn't seem to work, but > http://stackoverflow.com/questions/3678515/very-strange-calling-window-locati... > seems that return false is standard for what we want? > > In the case of config.html, the previous/next buttons are actual buttons while > here it's a submit form, though I haven't tested using a regular button here vs > a "submit" button in config. > > https://codereview.chromium.org/1949763002/diff/20001/appengine/findit/templa... > appengine/findit/templates/try_job_dashboard.html:65: {% for > try_job_display_data in try_jobs_in_progress %} > On 2016/05/04 00:00:53, stgao wrote: > > Maybe show "No in-progress try-jobs" if the list is empty? > > Done. lgtm
lgtm with nits. https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/try_job_dashboard.py (right): https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:42: '%Y-%m-%d %H:%M:%S UTC') no need to remove micro seconds. https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:61: if start or not end: # pragma: no cover Why it is no covered? Maybe "pragma: no branch" instead? https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... File appengine/findit/templates/try_job_dashboard.html (right): https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:29: return false; maybe event.preventDefault() instead? the event has to be passed into the function although. https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:87: No try-jobs currently in progress How about the other three tables?
https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... File appengine/findit/handlers/try_job_dashboard.py (right): https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:42: '%Y-%m-%d %H:%M:%S UTC') On 2016/05/04 21:13:36, stgao wrote: > no need to remove micro seconds. Done. https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/handle... appengine/findit/handlers/try_job_dashboard.py:61: if start or not end: # pragma: no cover On 2016/05/04 21:13:36, stgao wrote: > Why it is no covered? Maybe "pragma: no branch" instead? Done. https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... File appengine/findit/templates/try_job_dashboard.html (right): https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:29: return false; On 2016/05/04 21:13:36, stgao wrote: > maybe event.preventDefault() instead? the event has to be passed into the > function although. preventDefault() works https://codereview.chromium.org/1949763002/diff/40001/appengine/findit/templa... appengine/findit/templates/try_job_dashboard.html:87: No try-jobs currently in progress On 2016/05/04 21:13:36, stgao wrote: > How about the other three tables? Done.
The CQ bit was checked by lijeffrey@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/1949763002/#ps60001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1949763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1949763002/60001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Adding try job dashboard For sample dashboard, refer to: https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/try-job-dashboard BUG=608896 ========== to ========== [Findit] Adding try job dashboard For sample dashboard, refer to: https://lijeffrey-dot-findit-for-me.appspot.com/waterfall/try-job-dashboard BUG=608896 Committed: https://chromium.googlesource.com/infra/infra/+/ac711279b23ea8f985d2a1c2fe84a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/infra/infra/+/ac711279b23ea8f985d2a1c2fe84a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
