Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2224173002:
Added different colors for success/failure and code search. (Closed)
|
Created:
4 years, 4 months ago by BigBossZhiling Modified:
4 years, 4 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/build.git@master Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionAdded different colors for success/failure and enabled code search.
Success will be in green. Failure will be in red. Other status will be
in other colors. Also added code search functionality when clicking
test_name.
BUG=
Committed: https://chromium.googlesource.com/chromium/tools/build/+/fb7d4a217be54db25a24736b3684c4cecdea8fc1
Patch Set 1 #Patch Set 2 : fixes #
Total comments: 12
Patch Set 3 : addressing the comment #Patch Set 4 : fixes #
Total comments: 5
Patch Set 5 : fixed relative path #Patch Set 6 : fixes #
Total comments: 8
Patch Set 7 : fixes #
Total comments: 1
Patch Set 8 : fixed relative directory #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== Added different colors for success/failure and code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= ========== to ========== Added different colors for success/failure and code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= ==========
hzl@google.com changed reviewers: + jbudorick@chromium.org, yolandyan@chromium.org
Description was changed from ========== Added different colors for success/failure and code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= ========== to ========== Added different colors for success/failure and enabled code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= ==========
quick notice https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/main.html (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/main.html:5: <link rel="stylesheet" href="https://uberchromegw.corp.google.com/i/internal.client.clank/default.css" type="text/css"> hmm, you should to use relative path in your actual result page. this is open source code review
https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/table.html (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/table.html:18: {% if cell.is_pre %} I like this better! https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:40: def code_search(test): nice, this is much cleaner than my idea of using javascript to do it https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:42: return 'https://cs.corp.google.com/search/?q=%s&type=cs' % search this is a bit tricky, because internal links can't be shown in a open sourced script, you will have to create an argument in the script to pass in the link if it's downstream, and if it's upstream, the script can use the default upstream code search link here (https://cs.chromium.org) https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:46: data = [{'data': result['name'], 'class': 'left', nit: extra space https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:55: suite_name = result['name'][:result['name'].index('#')] nit: add the comment here to separate constructions of test_row_list and suite_row_dict extra nit: test_case_path = result['name'] test_suite_name = test_case_path[:test_case_path.index('#')]
I noticed a problem with using relative directory for the default.css file. Relative directory is relative to the directory of the html, not table or main.html, but the html created by test_results_presentation.py. Its directory is arbitrary and input by the users. So for example, I arbitrate the --htmll-file to be /tmp/zhiling_testing.html, there is no way that the relative directory in main.html is able to use relative directory to fetch the default.css. Any thoughts? https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/main.html (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/main.html:5: <link rel="stylesheet" href="https://uberchromegw.corp.google.com/i/internal.client.clank/default.css" type="text/css"> On 2016/08/08 22:01:00, the real yoland wrote: > hmm, you should to use relative path in your actual result page. > this is open source code review Done. https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/table.html (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/table.html:18: {% if cell.is_pre %} On 2016/08/08 22:34:50, the real yoland wrote: > I like this better! Done. https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:40: def code_search(test): On 2016/08/08 22:34:51, the real yoland wrote: > nice, this is much cleaner than my idea of using javascript to do it Acknowledged. https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:42: return 'https://cs.corp.google.com/search/?q=%s&type=cs' % search On 2016/08/08 22:34:51, the real yoland wrote: > this is a bit tricky, because internal links can't be shown in a open sourced > script, you will have to create an argument in the script to pass in the link if > it's downstream, and if it's upstream, the script can use the default upstream > code search link here (https://cs.chromium.org) Done. https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:46: data = [{'data': result['name'], 'class': 'left', On 2016/08/08 22:34:51, the real yoland wrote: > nit: extra space Done. https://codereview.chromium.org/2224173002/diff/20001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:55: suite_name = result['name'][:result['name'].index('#')] On 2016/08/08 22:34:50, the real yoland wrote: > nit: add the comment here to separate constructions of test_row_list and > suite_row_dict > > extra nit: > test_case_path = result['name'] > test_suite_name = test_case_path[:test_case_path.index('#')] Done.
I noticed a problem with using relative directory for the default.css file. Relative directory is relative to the directory of the html, not table or main.html, but the html created by test_results_presentation.py. Its directory is arbitrary and input by the users. So for example, I arbitrate the --htmll-file to be /tmp/zhiling_testing.html, there is no way that the relative directory in main.html is able to use relative directory to fetch the default.css. Any thoughts?
Thx for clearance on relative paths. Please take a look.
As discussed with +hzl, this isn't a problem for the page since the css page is already provided in the url's top level. https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/table.html (right): https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/table.html:21: <a onclick="window.open('{{cell.link}}');">{{cell.data}}</a> maybe just `<a href={{cell.link}}>` https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:98: 'table_class' : 'info', I think all the table class are just info, right?
https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/table.html (right): https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/table.html:21: <a onclick="window.open('{{cell.link}}');">{{cell.data}}</a> On 2016/08/11 06:35:16, the real yoland wrote: > maybe just `<a href={{cell.link}}>` href I think will replace the current page with the new page. But with window.open, it will open up a new page and leave the old page still there. Do we want the old page to be still open? https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:98: 'table_class' : 'info', On 2016/08/11 06:35:16, the real yoland wrote: > I think all the table class are just info, right? Done.
lgtm https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/table.html (right): https://codereview.chromium.org/2224173002/diff/60001/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/table.html:21: <a onclick="window.open('{{cell.link}}');">{{cell.data}}</a> On 2016/08/11 at 06:49:45, BigBossZhiling wrote: > On 2016/08/11 06:35:16, the real yoland wrote: > > maybe just `<a href={{cell.link}}>` > > href I think will replace the current page with the new page. But with window.open, it will open up a new page and leave the old page still there. Do we want the old page to be still open? I see, you can use `<a href="abc.xyz" target="_blank"></a>`
hzl@google.com changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/main.html (right): https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/main.html:5: <link rel="stylesheet" href="../../../../../../../default.css" type="text/css"> can you explain... href="../../../../../../../default.css not sure what this points to. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:42: url = cs_base_url if cs_base_url else 'https://cs.chromium.org' Should probably either set http://cs.chromium.org as the default to the script --cs-base-url arg or default to this function arg. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:61: suite_name = test_case_path[:test_case_path.index('#')] super nit: I usually this this done by... test_case_path.split('#')[0] and I think it makes it clearer. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:123: parser.add_argument('--cs-base-url', help='Base url for code search.') consider adding cs.chromium.org the default arg value.
https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/template/main.html (right): https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/template/main.html:5: <link rel="stylesheet" href="../../../../../../../default.css" type="text/css"> On 2016/08/12 18:58:52, mikecase wrote: > can you explain... > > href="../../../../../../../default.css > > not sure what this points to. This took me some time to figure out. This does not point to a real directory, but a virtual one. The html is located at this url, 'https://uberchromegw.corp.google.com/i/internal.client.clank/builders/test-phone/builds/2010/steps/process%20results%20for%20chrome_test_apk.Read%20detail.html/logs/result_details'. The default.css is here: 'https://uberchromegw.corp.google.com/i/internal.client.clank/default.css'. So we have to go back by 7 directories. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... File scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py (right): https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:42: url = cs_base_url if cs_base_url else 'https://cs.chromium.org' On 2016/08/12 18:58:52, mikecase wrote: > Should probably either set > > http://cs.chromium.org > > as the default to the script --cs-base-url arg or default to this function arg. Done. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:61: suite_name = test_case_path[:test_case_path.index('#')] On 2016/08/12 18:58:52, mikecase wrote: > super nit: > > I usually this this done by... > > test_case_path.split('#')[0] > > and I think it makes it clearer. Done. https://codereview.chromium.org/2224173002/diff/50004/scripts/slave/recipe_mo... scripts/slave/recipe_modules/chromium_android/resources/test_results_presentation.py:123: parser.add_argument('--cs-base-url', help='Base url for code search.') On 2016/08/12 18:58:52, mikecase wrote: > consider adding http://cs.chromium.org the default arg value. Done.
https://codereview.chromium.org/2224173002/diff/110001/scripts/slave/recipe_m... File scripts/slave/recipe_modules/chromium_android/resources/template/main.html (right): https://codereview.chromium.org/2224173002/diff/110001/scripts/slave/recipe_m... scripts/slave/recipe_modules/chromium_android/resources/template/main.html:5: <link rel="stylesheet" href="../../../../../../../default.css" type="text/css"> :| Where does this default.css live?
Changed relative directory. If the master name is not specified, the program won't crash. So we are able to submit this cl first, and then submit another cl that adds master name as an argument to test_results_presentation.py.
lgtm
The CQ bit was checked by hzl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yolandyan@chromium.org Link to the patchset: https://codereview.chromium.org/2224173002/#ps130001 (title: "fixed relative directory")
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 ========== Added different colors for success/failure and enabled code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= ========== to ========== Added different colors for success/failure and enabled code search. Success will be in green. Failure will be in red. Other status will be in other colors. Also added code search functionality when clicking test_name. BUG= Committed: https://chromium.googlesource.com/chromium/tools/build/+/fb7d4a217be54db25a24... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/tools/build/+/fb7d4a217be54db25a24... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
