Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
503028:
Python script for dromaeo benchmark automation. (Closed)
|
Created:
11 years ago by pavel.podivilov Modified:
9 years, 7 months ago Reviewers:
Vitaly Repeshko CC:
pfeldman Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionPython script for dromaeo benchmark automation.
BUG=none
TEST=none
Patch Set 1 #Patch Set 2 : '' #
Total comments: 48
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 10
Patch Set 6 : '' #Messages
Total messages: 4 (0 generated)
Looks promising. -- Vitaly http://codereview.chromium.org/503028/diff/1001/1002 File tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py (right): http://codereview.chromium.org/503028/diff/1001/1002#newcode1 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:1: #!/usr/bin/python2.4 This should be "/usr/bin/evn python". http://codereview.chromium.org/503028/diff/1001/1002#newcode3 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:3: # Copyright 2008 Google Inc. All Rights Reserved. 2009 http://codereview.chromium.org/503028/diff/1001/1002#newcode21 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:21: import gdata.spreadsheet.service Do we require something installed to get this? If yes, please document it. http://codereview.chromium.org/503028/diff/1001/1002#newcode31 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:31: help="directory with your dromaeo files") Add a link to a place where one can get Dromaeo version that supports this runner. (Maybe in the toplevel comment for this file.) http://codereview.chromium.org/503028/diff/1001/1002#newcode34 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:34: parser.add_option("-p", "--password", dest="password", Maybe you can get this using no-echo input and save in some cookie file (readable only by owner)? http://codereview.chromium.org/503028/diff/1001/1002#newcode40 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:40: default="http://localhost:8080/index.html?dom&automated", Provide a flag for the port. 8080 may well be occupied by something else. http://codereview.chromium.org/503028/diff/1001/1002#newcode51 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:51: def KillProcessByName(process_name): I don't think this is the most natural interface to (later) implement on Linux. Can you hide windows-specific details a bit deeper? http://codereview.chromium.org/503028/diff/1001/1002#newcode56 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:56: if (match): () are not needed. http://codereview.chromium.org/503028/diff/1001/1002#newcode58 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:58: subprocess.Popen('taskkill /pid %s' % pid) You have to wait for the command to finish. There is a convenience subprocess.call() function to run something and wait for its termination. http://codereview.chromium.org/503028/diff/1001/1002#newcode61 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:61: class SpreadsheetWriter(object): Add a comment (with some ascii art maybe) describing the structure of the spreadsheet you're creating. http://codereview.chromium.org/503028/diff/1001/1002#newcode79 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:79: # create worksheet column titles c0, c1, ... Mention that titles are needed to speed up spreadsheet updates. http://codereview.chromium.org/503028/diff/1001/1002#newcode80 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:80: if (1): Huh? http://codereview.chromium.org/503028/diff/1001/1002#newcode93 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:93: global rootnode Doesn't seem to be used. http://codereview.chromium.org/503028/diff/1001/1002#newcode113 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:113: # google spreadsheet has just 20 columns Can you make blocks of code on lines 113--116, 125--131 methods of SpreadsheetWriter? http://codereview.chromium.org/503028/diff/1001/1002#newcode122 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:122: options.spreadsheet_title, columns_count) >80 chars. http://codereview.chromium.org/503028/diff/1001/1002#newcode123 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:123: blank_row = ['-' for i in range(columns_count)] Hint: '-' * columns_count should also work. http://codereview.chromium.org/503028/diff/1001/1002#newcode128 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:128: for i in range(options.run_count): Hint: use xrange instead of range, it creates an iterator instead of a list. http://codereview.chromium.org/503028/diff/1001/1002#newcode133 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:133: for browser in options.browsers: Move the body of the loop to BenchmarkBrowser() method? http://codereview.chromium.org/503028/diff/1001/1002#newcode135 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:135: # create browser title Another SpreadsheetWriter method? http://codereview.chromium.org/503028/diff/1001/1002#newcode137 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:137: spreadsheet_writer.InsertRow([browser_name, time.strftime('%d.%m.%Y %H:%M:%S')]) > 80 chars. http://codereview.chromium.org/503028/diff/1001/1002#newcode141 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:141: for run_number in range(options.run_count): See above. http://codereview.chromium.org/503028/diff/1001/1002#newcode143 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:143: time.sleep(3) # sleep a little to avoid process run/terminate problems Doesn't look nice at all. Can we avoid this? (Probably with some waiting?) http://codereview.chromium.org/503028/diff/1001/1002#newcode153 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:153: for test_data_json in server.json: Can you create a convenient wrapper class for the json result and move this processing there? http://codereview.chromium.org/503028/diff/1001/1002#newcode163 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:163: KillProcessByName(browser) I think here we should wait() for the browser process to terminate. http://codereview.chromium.org/503028/diff/1001/1002#newcode165 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:165: # insert test results into spreadsheet Yet another SpreadsheetWriter method?
http://codereview.chromium.org/503028/diff/1001/1002 File tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py (right): http://codereview.chromium.org/503028/diff/1001/1002#newcode1 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:1: #!/usr/bin/python2.4 On 2009/12/16 17:22:58, Vitaly wrote: > This should be "/usr/bin/evn python". Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode3 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:3: # Copyright 2008 Google Inc. All Rights Reserved. On 2009/12/16 17:22:58, Vitaly wrote: > 2009 Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode21 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:21: import gdata.spreadsheet.service On 2009/12/16 17:22:58, Vitaly wrote: > Do we require something installed to get this? If yes, please document it. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode31 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:31: help="directory with your dromaeo files") On 2009/12/16 17:22:58, Vitaly wrote: > Add a link to a place where one can get Dromaeo version that supports this > runner. (Maybe in the toplevel comment for this file.) Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode40 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:40: default="http://localhost:8080/index.html?dom&automated", On 2009/12/16 17:22:58, Vitaly wrote: > Provide a flag for the port. 8080 may well be occupied by something else. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode56 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:56: if (match): On 2009/12/16 17:22:58, Vitaly wrote: > () are not needed. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode58 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:58: subprocess.Popen('taskkill /pid %s' % pid) On 2009/12/16 17:22:58, Vitaly wrote: > You have to wait for the command to finish. There is a convenience > subprocess.call() function to run something and wait for its termination. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode61 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:61: class SpreadsheetWriter(object): On 2009/12/16 17:22:58, Vitaly wrote: > Add a comment (with some ascii art maybe) describing the structure of the > spreadsheet you're creating. Done. Format description is in the toplevel comment. http://codereview.chromium.org/503028/diff/1001/1002#newcode79 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:79: # create worksheet column titles c0, c1, ... On 2009/12/16 17:22:58, Vitaly wrote: > Mention that titles are needed to speed up spreadsheet updates. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode80 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:80: if (1): On 2009/12/16 17:22:58, Vitaly wrote: > Huh? Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode93 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:93: global rootnode On 2009/12/16 17:22:58, Vitaly wrote: > Doesn't seem to be used. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode113 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:113: # google spreadsheet has just 20 columns On 2009/12/16 17:22:58, Vitaly wrote: > Can you make blocks of code on lines 113--116, 125--131 methods of > SpreadsheetWriter? Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode122 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:122: options.spreadsheet_title, columns_count) On 2009/12/16 17:22:58, Vitaly wrote: > >80 chars. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode123 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:123: blank_row = ['-' for i in range(columns_count)] On 2009/12/16 17:22:58, Vitaly wrote: > Hint: '-' * columns_count should also work. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode128 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:128: for i in range(options.run_count): On 2009/12/16 17:22:58, Vitaly wrote: > Hint: use xrange instead of range, it creates an iterator instead of a list. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode133 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:133: for browser in options.browsers: On 2009/12/16 17:22:58, Vitaly wrote: > Move the body of the loop to BenchmarkBrowser() method? After refactoring loop body is not very large, and it uses quite a lot of local variables - browser, spreadsheet_writer, options, server. Probably it's better to leave it as is. http://codereview.chromium.org/503028/diff/1001/1002#newcode135 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:135: # create browser title On 2009/12/16 17:22:58, Vitaly wrote: > Another SpreadsheetWriter method? Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode137 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:137: spreadsheet_writer.InsertRow([browser_name, time.strftime('%d.%m.%Y %H:%M:%S')]) On 2009/12/16 17:22:58, Vitaly wrote: > > 80 chars. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode141 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:141: for run_number in range(options.run_count): On 2009/12/16 17:22:58, Vitaly wrote: > See above. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode143 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:143: time.sleep(3) # sleep a little to avoid process run/terminate problems On 2009/12/16 17:22:58, Vitaly wrote: > Doesn't look nice at all. Can we avoid this? (Probably with some waiting?) Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode153 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:153: for test_data_json in server.json: On 2009/12/16 17:22:58, Vitaly wrote: > Can you create a convenient wrapper class for the json result and move this > processing there? Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode163 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:163: KillProcessByName(browser) On 2009/12/16 17:22:58, Vitaly wrote: > I think here we should wait() for the browser process to terminate. Done. http://codereview.chromium.org/503028/diff/1001/1002#newcode165 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:165: # insert test results into spreadsheet On 2009/12/16 17:22:58, Vitaly wrote: > Yet another SpreadsheetWriter method? Done.
LGTM. http://codereview.chromium.org/503028/diff/5001/5002 File tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py (right): http://codereview.chromium.org/503028/diff/5001/5002#newcode7 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:7: Script runs dromaeo tests in browsers specified by --browsers switch It's called --browser and can be specified multiple times. http://codereview.chromium.org/503028/diff/5001/5002#newcode10 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:10: You'll need to install Google Data APIs Python Client Library to run this Format like this: Prerequisites: 1. Install Google Data APIs Python Client Library from ... 2. Checkout Dromaeo benchmark from ... 3. Create a spreadsheet ... http://codereview.chromium.org/503028/diff/5001/5002#newcode26 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:26: Please note, that spreadsheet should be created manually. Please add a copy-pasteable sample usage. http://codereview.chromium.org/503028/diff/5001/5002#newcode116 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:116: # search for the spreadsheet with title = spreadsheet_title Reformat comments to start from a capital letter and end with period. http://codereview.chromium.org/503028/diff/5001/5002#newcode123 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:123: self.worksheet_id = 'od6' What does magic "od6" mean?
http://codereview.chromium.org/503028/diff/5001/5002 File tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py (right): http://codereview.chromium.org/503028/diff/5001/5002#newcode7 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:7: Script runs dromaeo tests in browsers specified by --browsers switch On 2009/12/24 19:11:38, Vitaly wrote: > It's called --browser and can be specified multiple times. Done. http://codereview.chromium.org/503028/diff/5001/5002#newcode10 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:10: You'll need to install Google Data APIs Python Client Library to run this On 2009/12/24 19:11:38, Vitaly wrote: > Format like this: > Prerequisites: > 1. Install Google Data APIs Python Client Library from ... > 2. Checkout Dromaeo benchmark from ... > 3. Create a spreadsheet ... Done. http://codereview.chromium.org/503028/diff/5001/5002#newcode26 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:26: Please note, that spreadsheet should be created manually. On 2009/12/24 19:11:38, Vitaly wrote: > Please add a copy-pasteable sample usage. Done. http://codereview.chromium.org/503028/diff/5001/5002#newcode116 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:116: # search for the spreadsheet with title = spreadsheet_title On 2009/12/24 19:11:38, Vitaly wrote: > Reformat comments to start from a capital letter and end with period. Done. http://codereview.chromium.org/503028/diff/5001/5002#newcode123 tools/dromaeo_benchmark_runner/dromaeo_benchmark_runner.py:123: self.worksheet_id = 'od6' On 2009/12/24 19:11:38, Vitaly wrote: > What does magic "od6" mean? Replaced with worksheet feed. |
