Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829408:
Smarter "new interesting code" detection. (Closed)
|
Created:
8 years, 4 months ago by Emily Fortuna Modified:
8 years, 4 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionSmarter "new interesting code" detection.
Committed: https://code.google.com/p/dart/source/detail?r=11186
Patch Set 1 : #
Total comments: 23
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 13
Patch Set 5 : #
Messages
Total messages: 7 (0 generated)
https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/webdr... File tools/testing/webdriver_test_setup.py (right): https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/webdr... tools/testing/webdriver_test_setup.py:42: help="Don't install Firefox", action='store_true', default=False) These were changes you suggested in the previous CL that somehow managed to escape being committed previously.
https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:112: def _sync(self, revision_num=''): nit: we would usually use None for the default value and test with revision_num is None https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:129: def sync_and_build(self, suites, revision_num=''): Google style is to use MixedCase for functions and methods. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:145: DART_REPO_LOC, 'tools', 'get_archive.py')) \ Better style is to put the whole condition in parens to avoid the backslash. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:196: def get_svn_log_data(revision): The function is not really getting log data. it is getting some partially processed lines from the log file. I would change this to two totally reusable global functions (they don't need any TestRunner state) (1) a small function that reads the svn log text for a revision (2) another that converts the text into a clean list of filenames, including the /branches/ prefix. You can test (2) on string literals (you do have unit tests somewhere, don't you?) https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:216: changed_files += [result.replace('/branches/bleeding_edge/dart/', '')] Don't strip of 'dart', you don't want to miss /branches/bleeding_edge/deps/... Some log files contain extra per-file info: A /branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/RecoveryParserTest.java (from /branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/ParserRecoveryTest.java:9962) It is probably simplest to pick out the text from /branches/ to the end of the filename (space or $) with a regexp https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:219: def has_perf_affecting_results(results): 'results' is kind of generic. Is this a list of lines, or a list of filenames? https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:229: [tokens[-1].startswith(item) for item in no_effect], False): If the filenames are cleanly extracted, and the no_effect list has the full /branches/... prefix, this becomes much simpler: def IsSafeFile(file): return any(file.startswith(prefix) for prefix in no_effect) return not all(IsSafeFile(file) for file in files) https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:241: if len(done_cls) == 0: idoimatic python tests for non-empty list like this: if done_cls: last_done_cl = int(done_cls[-1]) else: last_done_cl = EARLIEST_REVISION https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:505: if len(self.test.values_dict[platform][variant][benchmark]) == 0: len(x) == 0 --> not x if not self.test.values_dict[platform][variant][benchmark]: https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:511: len(self.test.values_dict[platform][variant][benchmark]) - 1]) Python has negative indexes from the end, so indexing by -1 give the last element. x[len(x)-1] --> x[-1]
Made the change in this CL to actually only go forward for now. I left the other logic commented out, so that it would be in the repo (since it's "new" in this CL), but if you want me to take it out, too, I can. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:112: def _sync(self, revision_num=''): On 2012/08/18 23:08:55, sra1 wrote: > nit: we would usually use None for the default value and test with revision_num > is None Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:129: def sync_and_build(self, suites, revision_num=''): On 2012/08/18 23:08:55, sra1 wrote: > Google style is to use MixedCase for functions and methods. Not for methods: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... and there seems to be some vagueness here: http://www.chromium.org/developers/coding-style https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:145: DART_REPO_LOC, 'tools', 'get_archive.py')) \ On 2012/08/18 23:08:55, sra1 wrote: > Better style is to put the whole condition in parens to avoid the backslash. Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:196: def get_svn_log_data(revision): On 2012/08/18 23:08:55, sra1 wrote: > The function is not really getting log data. it is getting some partially > processed lines from the log file. > > I would change this to two totally reusable global functions (they don't need > any TestRunner state) > > (1) a small function that reads the svn log text for a revision > (2) another that converts the text into a clean list of filenames, including the > /branches/ prefix. > > You can test (2) on string literals (you do have unit tests somewhere, don't > you?) Made into a global function. No unit tests for this (yet). :-( https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:216: changed_files += [result.replace('/branches/bleeding_edge/dart/', '')] On 2012/08/18 23:08:55, sra1 wrote: > Don't strip of 'dart', you don't want to miss /branches/bleeding_edge/deps/... > > Some log files contain extra per-file info: > A > /branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/RecoveryParserTest.java > (from > /branches/bleeding_edge/dart/editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/ParserRecoveryTest.java:9962) > > It is probably simplest to pick out the text from /branches/ to the end of the > filename (space or $) with a regexp Sort-of-done (with a TODO)... This requires me to have a different checkout of the repo instead of "gclient config https://dart.googlecode.com/svn/branches/bleeding_edge/deps/all.deps" and then "gclient sync". What's the proper method for the different checkout? https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:219: def has_perf_affecting_results(results): On 2012/08/18 23:08:55, sra1 wrote: > 'results' is kind of generic. > Is this a list of lines, or a list of filenames? Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:229: [tokens[-1].startswith(item) for item in no_effect], False): On 2012/08/18 23:08:55, sra1 wrote: > If the filenames are cleanly extracted, and the no_effect list has the full > /branches/... prefix, this becomes much simpler: > > > def IsSafeFile(file): > return any(file.startswith(prefix) for prefix in no_effect) > return not all(IsSafeFile(file) for file in files) Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:241: if len(done_cls) == 0: On 2012/08/18 23:08:55, sra1 wrote: > idoimatic python tests for non-empty list like this: > > if done_cls: > last_done_cl = int(done_cls[-1]) > else: > last_done_cl = EARLIEST_REVISION Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:505: if len(self.test.values_dict[platform][variant][benchmark]) == 0: On 2012/08/18 23:08:55, sra1 wrote: > len(x) == 0 --> not x > if not self.test.values_dict[platform][variant][benchmark]: Done. https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:511: len(self.test.values_dict[platform][variant][benchmark]) - 1]) On 2012/08/18 23:08:55, sra1 wrote: > Python has negative indexes from the end, so indexing by -1 give the last > element. > > x[len(x)-1] --> x[-1] > Done.
Changed capitalization from Google External Style guide to the Chromium Style guide at sra's request.
bump, PTAL?
lgtm https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:129: def sync_and_build(self, suites, revision_num=''): On 2012/08/20 19:59:05, Emily Fortuna wrote: > On 2012/08/18 23:08:55, sra1 wrote: > > Google style is to use MixedCase for functions and methods. > > Not for methods: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... > > and there seems to be some vagueness here: > http://www.chromium.org/developers/coding-style The second link says "Python code should follow PEP-8, except that Chromium uses two-space indentation instead of four-space indentation, and it uses MixedCase for method names and function names instead of lower_case_with_underscores." https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:191: def HasInterestingCode(self, past_revision_num=None): Why is it called 'past_revision_num'? https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:196: therefore the second element in the tuple will be None.)""" Might be clearer if you said something like Returns: (False, None): .... (True, past_revision_num): (True, other_revision_num): It might be simpler to return a revision number or None. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:201: 'samples/calculator', 'samples/chat', 'samples/clock', In my quick hack I has a 'yes' list and a 'no' list. If the file is under an entry in the 'yes' list it was in. Thus all the samples were taken care of my yes = [ ..., 'samples/third_party/dromaeo', ...] no = [ ..., 'sample', ...] https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:212: def GetChangedFileList(revision): You could just call it GetFileList. One might assume changed == modified, but we are intersted in added, moved and deleted files too. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:234: # that, we need to change our original checkout. So in preparation for being able to match the DEPS changes, you could keep the dart prefix. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:260: results = GetChangedFileList(latest_interesting_server_rev) better name: results -> filelist or files or filenames https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1112: interesting_code_results = runner.HasInterestingCode() Destructuring is often clearer and prevents bugs if you accidentally return the wrong arity tuple (is_interesting, interesting_revision) = runner.Has... if is_interersting: runner.RunTestSequence(interesting_revision) An alternative is to return the interesting revision number, or None.
https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/2001/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:129: def sync_and_build(self, suites, revision_num=''): On 2012/08/21 22:59:36, sra1 wrote: > On 2012/08/20 19:59:05, Emily Fortuna wrote: > > On 2012/08/18 23:08:55, sra1 wrote: > > > Google style is to use MixedCase for functions and methods. > > > > Not for methods: > > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Naming... > > > > and there seems to be some vagueness here: > > http://www.chromium.org/developers/coding-style > > The second link says "Python code should follow PEP-8, except that Chromium uses > two-space indentation instead of four-space indentation, and it uses MixedCase > for method names and function names instead of lower_case_with_underscores." Yea, I was reading the wrong section of the style guide like we discussed. Fixed. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:191: def HasInterestingCode(self, past_revision_num=None): On 2012/08/21 22:59:36, sra1 wrote: > Why is it called 'past_revision_num'? Silly historical reasons. Fixed. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:196: therefore the second element in the tuple will be None.)""" On 2012/08/21 22:59:36, sra1 wrote: > Might be clearer if you said something like > > Returns: > (False, None): .... > (True, past_revision_num): > (True, other_revision_num): > > It might be simpler to return a revision number or None. Done. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:212: def GetChangedFileList(revision): On 2012/08/21 22:59:36, sra1 wrote: > You could just call it GetFileList. > One might assume changed == modified, but we are intersted in added, moved and > deleted files too. Done. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:234: # that, we need to change our original checkout. On 2012/08/21 22:59:36, sra1 wrote: > So in preparation for being able to match the DEPS changes, you could keep the > dart prefix. Done. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:260: results = GetChangedFileList(latest_interesting_server_rev) On 2012/08/21 22:59:36, sra1 wrote: > better name: results -> filelist or files or filenames Done. https://chromiumcodereview.appspot.com/10829408/diff/9003/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1112: interesting_code_results = runner.HasInterestingCode() On 2012/08/21 22:59:36, sra1 wrote: > Destructuring is often clearer and prevents bugs if you accidentally return the > wrong arity tuple > > (is_interesting, interesting_revision) = runner.Has... > if is_interersting: > runner.RunTestSequence(interesting_revision) > > An alternative is to return the interesting revision number, or None. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
