Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836003002:
[Findit] Rewrite UI page. (Closed)
Description[Findit] Rewrite UI page.
BUG=598507
Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20(1)/builds/13844
Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20(1)/builds/24142
Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20(1)/builds/13723
Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20(dbg)(1)/builds/52854#
Committed: https://chromium.googlesource.com/infra/infra/+/8608f9dc4f05a378f012e9a2b99c064f945b3cfe
Patch Set 1 #
Total comments: 50
Patch Set 2 : . #
Total comments: 3
Depends on Patchset: Messages
Total messages: 18 (10 generated)
Patchset #2 (id:20001) has been deleted
chanli@chromium.org changed reviewers: + lijeffrey@chromium.org, stgao@chromium.org
PTAL
Description was changed from ========== [Findit] Rewrite UI page. BUG=none ========== to ========== [Findit] Rewrite UI page. BUG=none samples: Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ==========
Description was changed from ========== [Findit] Rewrite UI page. BUG=none samples: Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ========== to ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ==========
Description was changed from ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ========== to ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ==========
Description was changed from ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ========== to ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ==========
https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:97: findit.tryjobStatusMessageTable = {{status_message_map | tojson | safe}} nit: table -> map. In a html, table has its special meaning I guess. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:163: function displayCommonCells(step_name, category, index, tests, first_failure, last_pass, rowspan) { name nit: displayCommonCells -> generateCommonClessForSuspectedCL or something more explicit? We don't display anything yet in this func. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:169: rowString += '<li><div title="' + tests[j] + '" class="truncate">' + tests[j] + '</div></li>'; Why an div? Could it be just an <li>? https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:192: clString += '<td>' + cl.repo_name + '</td>'; We don't show the repo_name above, so let's do the same here. After we support sub-CLs in DEPS rolls, we could add it back. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:207: function displayHeuristicCulpritCls(suspectedCls, supported){ name nit: no display in this func yet. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:210: if (suspectedCls.length > 0){ style nit: space ") {" https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:226: function displayDeterminedResult(step_name, results) { nit: DeterminedResult-> ReliableFailures https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:237: var build_number = result.try_job.try_job_key.split('/')[2]; A comment on example of the try_job_key and what does the build number refer to. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:239: tableString += '<td rowspan="' + rowspan + '"><a href="' + tryJobCulprit.review_url + '">' + tryJobCulprit.commit_position || tryJobCulprit.revision + '</a></td>'; Is it review_url? If no review url, we default to git log url. This seems a potential bug. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:242: tableString += '<li>Tests are reliable based on result of swarming rerun <a href="' + tryJob.task_url+ '">' + tryJob.task_id + '</a></li>'; The message seems a bit long, we'd better shorten it. Reliable failure: <a href='url/to/swarming/task'>swarming rerun</a> https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:252: if (!jQuery.isEmptyObject(results)) { Should we check this at the beginning instead? Same for the other two categories below. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:258: function displayFlakyResult(step_name, results) { name nit: FlakyResult -> flakyfailures. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:278: function displayUndeterminedResult(step_name, results) { name nit: UnderterminedResult -> UnclassifiedFailures. Also update var naming and id naming. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:328: } Give an alert to file bug for the "else" case? https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:402: <b>Determined Results</b> Reliable failures https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:407: <th rowspan="3" title="Failed test name">Test(s)</th> How about setting the width of the tests? https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:434: <b>Flaky Results</b> Flaky failures. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:460: <b>Undetermined Results</b> Unclassified failures.
https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:171: if(tests.length > 5) { nit: space after if https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:172: rowString += '<div id="list-' + category + '-' + index + '" class="not-display">'; nit: + index + <remove 1 space> https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:217: if(supported == false) { nit: space after if https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:217: if(supported == false) { why not if (!supported)? https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:228: $.each(results, function(index, result){ nit: space before { https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:442: <th rowspan="2" title="link to swarming rerun">Swaring Rerun</th> Swaring -> Swarming https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:443: <th colspan="5">Suspected CLs</th> Maybe also add "Heuristic Analysis Result" under Suspected CL so it is clear to the sheriff that anything found here was from heuristic-based analysis just like "Determined Results". Same with "Undetermined Results"
https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:97: findit.tryjobStatusMessageTable = {{status_message_map | tojson | safe}} On 2016/03/29 00:26:06, stgao wrote: > nit: table -> map. > > In a html, table has its special meaning I guess. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:163: function displayCommonCells(step_name, category, index, tests, first_failure, last_pass, rowspan) { On 2016/03/29 00:26:06, stgao wrote: > name nit: displayCommonCells -> generateCommonClessForSuspectedCL or something > more explicit? > > We don't display anything yet in this func. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:169: rowString += '<li><div title="' + tests[j] + '" class="truncate">' + tests[j] + '</div></li>'; On 2016/03/29 00:26:06, stgao wrote: > Why an div? Could it be just an <li>? This div is to truncate the test name if it's too long. I can't do it with <li> https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:171: if(tests.length > 5) { On 2016/03/29 00:49:44, lijeffrey wrote: > nit: space after if Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:172: rowString += '<div id="list-' + category + '-' + index + '" class="not-display">'; On 2016/03/29 00:49:44, lijeffrey wrote: > nit: + index + <remove 1 space> Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:192: clString += '<td>' + cl.repo_name + '</td>'; On 2016/03/29 00:26:06, stgao wrote: > We don't show the repo_name above, so let's do the same here. > > After we support sub-CLs in DEPS rolls, we could add it back. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:207: function displayHeuristicCulpritCls(suspectedCls, supported){ On 2016/03/29 00:26:05, stgao wrote: > name nit: no display in this func yet. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:210: if (suspectedCls.length > 0){ On 2016/03/29 00:26:06, stgao wrote: > style nit: space ") {" Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:217: if(supported == false) { On 2016/03/29 00:49:44, lijeffrey wrote: > why not if (!supported)? Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:217: if(supported == false) { On 2016/03/29 00:49:45, lijeffrey wrote: > nit: space after if Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:226: function displayDeterminedResult(step_name, results) { On 2016/03/29 00:26:06, stgao wrote: > nit: DeterminedResult-> ReliableFailures Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:228: $.each(results, function(index, result){ On 2016/03/29 00:49:44, lijeffrey wrote: > nit: space before { Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:237: var build_number = result.try_job.try_job_key.split('/')[2]; On 2016/03/29 00:26:05, stgao wrote: > A comment on example of the try_job_key and what does the build number refer to. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:239: tableString += '<td rowspan="' + rowspan + '"><a href="' + tryJobCulprit.review_url + '">' + tryJobCulprit.commit_position || tryJobCulprit.revision + '</a></td>'; On 2016/03/29 00:26:06, stgao wrote: > Is it review_url? If no review url, we default to git log url. > > This seems a potential bug. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:242: tableString += '<li>Tests are reliable based on result of swarming rerun <a href="' + tryJob.task_url+ '">' + tryJob.task_id + '</a></li>'; On 2016/03/29 00:26:06, stgao wrote: > The message seems a bit long, we'd better shorten it. > > Reliable failure: <a href='url/to/swarming/task'>swarming rerun</a> Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:252: if (!jQuery.isEmptyObject(results)) { On 2016/03/29 00:26:06, stgao wrote: > Should we check this at the beginning instead? > > Same for the other two categories below. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:258: function displayFlakyResult(step_name, results) { On 2016/03/29 00:26:06, stgao wrote: > name nit: FlakyResult -> flakyfailures. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:278: function displayUndeterminedResult(step_name, results) { On 2016/03/29 00:26:06, stgao wrote: > name nit: UnderterminedResult -> UnclassifiedFailures. > > Also update var naming and id naming. I'll go with unreliableFailures. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:328: } On 2016/03/29 00:26:06, stgao wrote: > Give an alert to file bug for the "else" case? Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:402: <b>Determined Results</b> On 2016/03/29 00:26:06, stgao wrote: > Reliable failures Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:407: <th rowspan="3" title="Failed test name">Test(s)</th> On 2016/03/29 00:26:06, stgao wrote: > How about setting the width of the tests? Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:434: <b>Flaky Results</b> On 2016/03/29 00:26:06, stgao wrote: > Flaky failures. Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:442: <th rowspan="2" title="link to swarming rerun">Swaring Rerun</th> On 2016/03/29 00:49:45, lijeffrey wrote: > Swaring -> Swarming Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:443: <th colspan="5">Suspected CLs</th> On 2016/03/29 00:49:44, lijeffrey wrote: > Maybe also add "Heuristic Analysis Result" under Suspected CL so it is clear to > the sheriff that anything found here was from heuristic-based analysis just like > "Determined Results". Same with "Undetermined Results" Done. https://codereview.chromium.org/1836003002/diff/1/appengine/findit/templates/... appengine/findit/templates/build_failure.html:460: <b>Undetermined Results</b> On 2016/03/29 00:26:06, stgao wrote: > Unclassified failures. Done.
Description was changed from ========== [Findit] Rewrite UI page. BUG=none Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ========== to ========== [Findit] Rewrite UI page. BUG=598507 Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ==========
Patchset #2 (id:40001) has been deleted
lgtm https://codereview.chromium.org/1836003002/diff/60001/appengine/findit/templa... File appengine/findit/templates/build_failure.html (right): https://codereview.chromium.org/1836003002/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:217: if (!supported) { ``supported`` could be null for legacy data? Jeff, did you make that change to tag whether a step is supported or not? https://codereview.chromium.org/1836003002/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:227: if (!jQuery.isEmptyObject(results)) { Alternative: if (jQuery.isEmptyObject(results)) return; Same for others. https://codereview.chromium.org/1836003002/diff/60001/appengine/findit/templa... appengine/findit/templates/build_failure.html:283: function displayunclassifiedFailures(step_name, results) { nit: unclassified -> Unclassified.
The CQ bit was checked by chanli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836003002/60001
Message was sent while issue was closed.
Description was changed from ========== [Findit] Rewrite UI page. BUG=598507 Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... ========== to ========== [Findit] Rewrite UI page. BUG=598507 Determined Result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Flaky: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Undetermined result: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Complicated case: https://chanli-test-dot-findit-for-me.appspot.com/build-failure?url=https://b... Committed: https://chromium.googlesource.com/infra/infra/+/8608f9dc4f05a378f012e9a2b99c0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/infra/infra/+/8608f9dc4f05a378f012e9a2b99c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
