Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1216033009:
Updated script to capture useful coverage stats. (Closed)
|
Created:
5 years, 5 months ago by estevenson1 Modified:
5 years, 4 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded coverage stats and report generation.
Sample Usage:
build/android/coverage.py -v --out <output file path> --emma-dir <EMMA file directory> --lines-for-coverage-file <path to file containing lines for coverage>
This CL adds the ability to generate code coverage reports for Java code. To use this script, a JSON file should exist that maps file paths to lists of integers, representing the lines that have changed for each file (i.e. incremental changes).
The generated report contains overall coverage, coverage for the lines specified in the input file, and line by line coverage for each file included in the input file.
BUG=501536
Committed: https://crrev.com/67aa871b52c94f30b52ef19fa71388a54fc3410f
Cr-Commit-Position: refs/heads/master@{#341437}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Small refactor. #
Total comments: 25
Patch Set 3 : Updates based on review. #
Total comments: 4
Patch Set 4 : Addressed jbudorick's comments. #Messages
Total messages: 14 (3 generated)
estevenson@google.com changed reviewers: + mikecase@chromium.org
https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:449: ValueError: An improperly formatted |line_coverage_file| was supplied. Need to update this docstring. Function doesn't raise errors anymore.
https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py#n... build/android/coverage.py:258: """Encapsulates coverage operations related to EMMA files. Probably change the description to be more about stats. "coverage operations" might be a little too vague. https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py#n... build/android/coverage.py:263: Additionally, this class stores the information needed to correlate EMMA HTML I like this comment but it might not belong here. Maybe move it to the _GetSourceFileToEmmaFileDict function instead? https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:194: full_package_name = '%s.%s' % (package_name, coverage_file_element.text) coverage_file_element.text is the class names? correct? if so, maybe rename coverage_file_element, to class_name_elements or something if that makes sense. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:195: package_to_emma[full_package_name] = emma_coverage_file_path It seems like it would probably be this way, but are you sure there is only ever one emma file per java class. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:216: class _EmmaCoverageStats(object): I still think this interface is a little confusing. I think there are just too many functions that sound super similar. For example, there is both GetCoverageReportForLines ("Gets code coverage stats for a given set of LineCoverage objects.") and GetStatsForLines ("Gets coverage stats for a list of LineCoverage objects and line numbers.") which sound almost identical. Additionally, there is a _GetCoverageStatusForFile which "Gets a list LineCoverage objects corresponding to the given file path.". The function sounds like it should be called _GetCoverageForFile or something instead. def GetCoverageDictForFiles(self, lines_for_coverage): """Returns a dict containing detailed coverage info for all files. def GetCoverageReportForLines(self, line_coverage, lines): """Gets code coverage stats for a given set of LineCoverage objects. def GetStatsForLines(self, line_coverage, line_numbers=None): """Gets coverage stats for a list of LineCoverage objects and line numbers. def GenerateCoverageReport(line_coverage_file, out_file_path, coverage_dir): """Generates a coverage report for a given set of lines. def _GetCoverageStatusForFile(self, file_path): """Gets a list LineCoverage objects corresponding to the given file path. def _GetSourceFileToEmmaFileDict(self, files): """Gets a dict used to correlate Java source files with EMMA HTML files. I think I suggested this before, but why not have 1 public function, GetCoverageStats(lines=None), where lines is an optional (if None it just returns all the coverage info from emma as a big python dict. if not None, it only returns coverage info for those specific liens.) If you really wanted more, you could add 1 additional public function GetCoverageStatsSummary(lines=None) which would only return 1 dict with YY/ZZ lines covered (either total or per file.) https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:233: # Regular expression to get package name from Java package statement. super nit: two spaces btw "expression" and "to" https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:235: RE_PACKAGE_MATCH_GROUP = 'package' If you are going to name the group. I would just rewrite it like... RE_PACKAGE_MATCH_GROUP = 'package' RE_PACKAGE = re.compile(r'package (?P<%s>[\w.]*);' % RE_PACKAGE_MATCH_GROUP) https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:252: lines_for_coverage: A dict mapping Java source file paths to lists of This wording is confusing. I would just simplify it with something like... lines_for_coverage: A dict mapping Java source file paths to lists of line numbers. |_EmmaCoverageStats| will perform coverage analysis on this subset of lines separately. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:255: this subset of files/lines separately. About, In addition to overall coverage stats, _EmmaCoverageStats will perform coverage analysis on this subset of files/lines separately. I would say that if lines_for_coverage is given, only coverage stats on those particular lines should be computed. If you wanted both overall coverage and coverage on some lines, I think it would make sense to call the function twice GetCoverageDictForFiles(lines_for_coverage=my_lines_for_coverage) # Specific line coverage. GetCoverageDictForFiles(lines_for_coverage=None) # Overall https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:349: """Gets a list LineCoverage objects corresponding to the given file path. nit: s/"Gets a list"/"Gets a list of" https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:405: return (os.path.splitext(file_path)[1] == '.java' Might want to add a logging.debug statement if some file is going to be ignored by this method. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:417: ex. org.chromium.chrome.Activity.java or None if there is no package super nit: Everywhere else you write "Example:" for examples. Might want to keep consistent. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:424: package = package_match.group(_EmmaCoverageStats.RE_PACKAGE_MATCH_GROUP) nit: I would prefer self.RE_PACKAGE and self.RE_PACKAGE_MATCH_GROUP over _EmmaCoverageStats.RE_PACKAGE and _EmmaCoverageStats.RE_PACKAGE_MATCH_GROUP here. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:459: sys.exit(0) I would probably just return here instead of sys.exit(0) (in case anyone wants to add more things to main() in the future after the call to GenerateCoverageReport or if another script wants to use GenerateCoverageReport for example)
estevenson@google.com changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py#n... build/android/coverage.py:258: """Encapsulates coverage operations related to EMMA files. On 2015/07/27 at 18:45:52, mikecase wrote: > Probably change the description to be more about stats. "coverage operations" might be a little too vague. Done. https://codereview.chromium.org/1216033009/diff/1/build/android/coverage.py#n... build/android/coverage.py:263: Additionally, this class stores the information needed to correlate EMMA HTML On 2015/07/27 at 18:45:52, mikecase wrote: > I like this comment but it might not belong here. Maybe move it to the _GetSourceFileToEmmaFileDict function instead? Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:194: full_package_name = '%s.%s' % (package_name, coverage_file_element.text) On 2015/07/27 at 18:45:52, mikecase wrote: > coverage_file_element.text is the class names? correct? > > if so, maybe rename coverage_file_element, to class_name_elements or something if that makes sense. Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:195: package_to_emma[full_package_name] = emma_coverage_file_path On 2015/07/27 at 18:45:52, mikecase wrote: > It seems like it would probably be this way, but are you sure there is only ever one emma file per java class. I wasn't able to find any cases where multiple emma files were generated for a single java class. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:216: class _EmmaCoverageStats(object): On 2015/07/27 at 18:45:52, mikecase wrote: > I still think this interface is a little confusing. I think there are just too many functions that sound super similar. > > For example, > there is both GetCoverageReportForLines ("Gets code coverage stats for a given set of LineCoverage objects.") and > GetStatsForLines ("Gets coverage stats for a list of LineCoverage objects and line numbers.") which sound almost identical. > > Additionally, there is a _GetCoverageStatusForFile which "Gets a list LineCoverage objects corresponding to the given file path.". > The function sounds like it should be called _GetCoverageForFile or something instead. > > def GetCoverageDictForFiles(self, lines_for_coverage): > """Returns a dict containing detailed coverage info for all files. > > def GetCoverageReportForLines(self, line_coverage, lines): > """Gets code coverage stats for a given set of LineCoverage objects. > > def GetStatsForLines(self, line_coverage, line_numbers=None): > """Gets coverage stats for a list of LineCoverage objects and line numbers. > > def GenerateCoverageReport(line_coverage_file, out_file_path, coverage_dir): > """Generates a coverage report for a given set of lines. > > def _GetCoverageStatusForFile(self, file_path): > """Gets a list LineCoverage objects corresponding to the given file path. > > def _GetSourceFileToEmmaFileDict(self, files): > """Gets a dict used to correlate Java source files with EMMA HTML files. > > > I think I suggested this before, but why not have 1 public function, GetCoverageStats(lines=None), > where lines is an optional (if None it just returns all the coverage info from emma as a big > python dict. if not None, it only returns coverage info for those specific liens.) > > If you really wanted more, you could add 1 additional public function GetCoverageStatsSummary(lines=None) > which would only return 1 dict with YY/ZZ lines covered (either total or per file.) Made some interface changes, let me know if it needs further tweaking. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:233: # Regular expression to get package name from Java package statement. On 2015/07/27 at 18:45:53, mikecase wrote: > super nit: two spaces btw "expression" and "to" Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:235: RE_PACKAGE_MATCH_GROUP = 'package' On 2015/07/27 at 18:45:52, mikecase wrote: > If you are going to name the group. I would just rewrite it like... > > RE_PACKAGE_MATCH_GROUP = 'package' > RE_PACKAGE = re.compile(r'package (?P<%s>[\w.]*);' % RE_PACKAGE_MATCH_GROUP) Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:252: lines_for_coverage: A dict mapping Java source file paths to lists of On 2015/07/27 at 18:45:53, mikecase wrote: > This wording is confusing. > > I would just simplify it with something like... > > lines_for_coverage: A dict mapping Java source file paths to lists of line numbers. |_EmmaCoverageStats| will perform coverage analysis on > this subset of lines separately. Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:255: this subset of files/lines separately. On 2015/07/27 at 18:45:52, mikecase wrote: > About, > > In addition to overall > coverage stats, _EmmaCoverageStats will perform coverage analysis on > this subset of files/lines separately. > > I would say that if lines_for_coverage is given, only coverage stats on those particular lines should be computed. > If you wanted both overall coverage and coverage on some lines, I think it would make sense to call the function twice > > GetCoverageDictForFiles(lines_for_coverage=my_lines_for_coverage) # Specific line coverage. > GetCoverageDictForFiles(lines_for_coverage=None) # Overall This method doesn't actually compute coverage for a file, it just calls the method that does for each file. The reason I do both at once in GetSummaryStats is that to fit with the Chrome extension, we need incremental and overall summaries for each file, there won't be a case where we only want the incremental summary (summary means total covered lines and total lines in this context). Let me know if you still want me to change GetSummaryStats to require a separate call for incremental and overall coverage summaries. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:349: """Gets a list LineCoverage objects corresponding to the given file path. On 2015/07/27 at 18:45:53, mikecase wrote: > nit: s/"Gets a list"/"Gets a list of" Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:405: return (os.path.splitext(file_path)[1] == '.java' On 2015/07/27 at 18:45:52, mikecase wrote: > Might want to add a logging.debug statement if some file is going to be ignored by this method. Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:417: ex. org.chromium.chrome.Activity.java or None if there is no package On 2015/07/27 at 18:45:53, mikecase wrote: > super nit: Everywhere else you write "Example:" for examples. Might want to keep consistent. Done. https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:424: package = package_match.group(_EmmaCoverageStats.RE_PACKAGE_MATCH_GROUP) On 2015/07/27 at 18:45:53, mikecase wrote: > nit: I would prefer self.RE_PACKAGE and self.RE_PACKAGE_MATCH_GROUP over _EmmaCoverageStats.RE_PACKAGE and _EmmaCoverageStats.RE_PACKAGE_MATCH_GROUP here. I did this because this is a static method (has no self). Is this not how to call a static method in python? Should I change this to be a regular method, or make it a global function? https://codereview.chromium.org/1216033009/diff/20001/build/android/coverage.... build/android/coverage.py:459: sys.exit(0) On 2015/07/27 at 18:45:52, mikecase wrote: > I would probably just return here instead of sys.exit(0) (in case anyone wants to add more things to main() in the future after the call to GenerateCoverageReport or if another script wants to use GenerateCoverageReport for example) Done.
https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... File build/android/emma_coverage_stats.py (right): https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... build/android/emma_coverage_stats.py:383: logging.warning('Skipping %s because it doesn\'t have a package ' nit: use double quotes for strings containing single quotes. https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... build/android/emma_coverage_stats.py:459: coverage_results = code_coverage.GetCoverageDictForAllFiles( This function doesn't seem to be in this CL...?
https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... File build/android/emma_coverage_stats.py (right): https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... build/android/emma_coverage_stats.py:383: logging.warning('Skipping %s because it doesn\'t have a package ' On 2015/07/31 at 20:21:14, jbudorick wrote: > nit: use double quotes for strings containing single quotes. Done. https://codereview.chromium.org/1216033009/diff/40001/build/android/emma_cove... build/android/emma_coverage_stats.py:459: coverage_results = code_coverage.GetCoverageDictForAllFiles( On 2015/07/31 at 20:21:14, jbudorick wrote: > This function doesn't seem to be in this CL...? Oops, was supposed to be GetCoverageDict.
lgtm
The CQ bit was checked by estevenson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1216033009/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1216033009/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/67aa871b52c94f30b52ef19fa71388a54fc3410f Cr-Commit-Position: refs/heads/master@{#341437} |
