Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211243016:
Added coverage script and tests. (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 script for Java code.
This CL contains the first part of a script that will be used to
generate code coverage stats for Java code. The coverage tool used,
EMMA, only provides line by line coverage information in the form of
HTML reports, so this CL handles parsing these files.
BUG=501536
Committed: https://crrev.com/cf902beeaf9f13167abe9d5631ae1d3be9303d92
Cr-Commit-Position: refs/heads/master@{#340833}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Fixed nits. #
Total comments: 14
Patch Set 3 : Removed lxml dependency. #
Total comments: 4
Patch Set 4 : Fixed nit. #Patch Set 5 : Renamed module to avoid any naming conflicts. #
Total comments: 2
Patch Set 6 : Removed references to lxml and added test to presubmit. #
Messages
Total messages: 29 (8 generated)
estevenson@google.com changed reviewers: + mikecase@chromium.org
Mostly nits. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:29: point number between [0.0 - 1.0]. s/"[0.0 - 1.0]"/"(0.0, 1.0)" https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:48: This is returned by the selector |XPATH_SELECT_PACKAGE_ELEMENTS|. nit: I would maybe just change .... "To get package links:" to "Package links:" because of the way the comment is written. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:50: To get class links: As above, maybe... s/"To get class links:"/"Class links:" https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:95: # Technically both child 1 and 2 contain the percentage covered for a line. nit: clarify this comment. What does "technically both child 1 and 2" mean? If it is that, literally both child 1 and 2 have the same information, I would just choose 1 and delete this comment (only makes things more confusing) https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:98: # The second child contains the original line of source code. So this says "second child" (index 1) and above you call it "child 1" (also index 1). I would stick to either calling them one way or the other. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:105: _CSS_TO_STATUS = {'c': LineCoverage.COVERED, nit: Would change formatting to be _CSS_TO_STATUS = { 'c': LineCoverage.COVERED, } https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:202: for link in package_a_elements if 'href' in link.attrib} nit: Would prefer format.... package_links = { os.path.join(self._base_dir, link.attrib['href']): link.text for link in package_a_elements if 'href' in link.attrib } https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:206: # The <a> elements that contain each class name in the current package and super nit: Kind of confusingly worked comments. Maybe change... " The <a> elements that contain..." to " These <a> elements contain..." https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:207: # the path of the file where the coverage info is stored for each class. Nit: capitalize "the" https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:213: emma_file_path = os.path.join(self._emma_files_path, what is the difference between package_emma_file_path and emma_file_path? https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:220: def _FindElements(self, file_path, **kwargs): Get rid of kwargs if you can. And just pass the xpath or something. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:237: return [] You should maybe just fail here? If this is expected to happen sometimes, you should at least log an error. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test.py File build/android/coverage_test.py (right): https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:266: def GetLineCoverageWithFakeElements(self, *args): Can you just change this to... GetLineCoverageWithFakeElements(self, html_elements) https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:295: for elem in root.iter('*'): nit: s/elem/element https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:301: def MockOpenForFunction(func, *args, **kwargs): Can you change this to... MockOpenForFunction(func, side_effects, **kwargs) or MockOpenForFunction(func, side_effects, *args, **kwargs) It seems like *args should have a name the way its used.
https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:29: point number between [0.0 - 1.0]. On 2015/07/07 at 17:12:33, mikecase wrote: > s/"[0.0 - 1.0]"/"(0.0, 1.0)" Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:48: This is returned by the selector |XPATH_SELECT_PACKAGE_ELEMENTS|. On 2015/07/07 at 17:12:32, mikecase wrote: > nit: > > I would maybe just change .... > > "To get package links:" > to > "Package links:" > because of the way the comment is written. Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:50: To get class links: On 2015/07/07 at 17:12:32, mikecase wrote: > As above, maybe... > s/"To get class links:"/"Class links:" Done, and fixed other occurrences. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:95: # Technically both child 1 and 2 contain the percentage covered for a line. On 2015/07/07 at 17:12:33, mikecase wrote: > nit: clarify this comment. What does "technically both child 1 and 2" mean? If it is that, literally both child 1 and 2 have the same information, I would just choose 1 and delete this comment (only makes things more confusing) Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:98: # The second child contains the original line of source code. On 2015/07/07 at 17:12:32, mikecase wrote: > So this says "second child" (index 1) and above you call it "child 1" (also index 1). I would stick to either calling them one way or the other. Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:105: _CSS_TO_STATUS = {'c': LineCoverage.COVERED, On 2015/07/07 at 17:12:32, mikecase wrote: > nit: Would change formatting to be > > _CSS_TO_STATUS = { > 'c': LineCoverage.COVERED, > } Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:202: for link in package_a_elements if 'href' in link.attrib} On 2015/07/07 at 17:12:33, mikecase wrote: > nit: Would prefer format.... > > package_links = { > os.path.join(self._base_dir, link.attrib['href']): link.text > for link in package_a_elements if 'href' in link.attrib > } Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:206: # The <a> elements that contain each class name in the current package and On 2015/07/07 at 17:12:33, mikecase wrote: > super nit: Kind of confusingly worked comments. > > Maybe change... > " The <a> elements that contain..." > to > " These <a> elements contain..." Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:213: emma_file_path = os.path.join(self._emma_files_path, On 2015/07/07 at 17:12:32, mikecase wrote: > what is the difference between package_emma_file_path and emma_file_path? package_emma_file_path is the path to the EMMA report html file that contains all the links to different class coverage files within the specific package. emma_file_path is simply a file path of an emma coverage file (i.e. the file that stores coverage information for a Java source file). Renamed emma_file_path to emma_coverage_file_path. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:220: def _FindElements(self, file_path, **kwargs): On 2015/07/07 at 17:12:33, mikecase wrote: > Get rid of kwargs if you can. And just pass the xpath or something. Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage.py#n... build/android/coverage.py:237: return [] On 2015/07/07 at 17:12:33, mikecase wrote: > You should maybe just fail here? If this is expected to happen sometimes, you should at least log an error. Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test.py File build/android/coverage_test.py (right): https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:266: def GetLineCoverageWithFakeElements(self, *args): On 2015/07/07 at 17:12:33, mikecase wrote: > Can you just change this to... > GetLineCoverageWithFakeElements(self, html_elements) Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:295: for elem in root.iter('*'): On 2015/07/07 at 17:12:33, mikecase wrote: > nit: s/elem/element Done. https://codereview.chromium.org/1211243016/diff/1/build/android/coverage_test... build/android/coverage_test.py:301: def MockOpenForFunction(func, *args, **kwargs): On 2015/07/07 at 17:12:33, mikecase wrote: > Can you change this to... > MockOpenForFunction(func, side_effects, **kwargs) or MockOpenForFunction(func, side_effects, *args, **kwargs) > > It seems like *args should have a name the way its used. Done.
a final few comments, but I think this CL looks good. I'm not an owner of build/android though, so jbudorick will have to take a look. non-owner lgtm with nits +jbudorick https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:150: # Parse string that contains percent covered: "83% line coverage ,,,". Is this supposed to be "83% line coverage ,,," or did you mean "83% line coverage ..." https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage_... File build/android/coverage_test.py (right): https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage_... build/android/coverage_test.py:287: def MakeElementsWithoutWhitespace(self, html_string): This seems a little weird. You could have you Html elements formatted like EXAMPLE_HTML = \ "<html>" + \ "example" + \ "</html>" so you dont get all the extra whitespace and don't need this function.
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:9: from lxml import html I'm somewhat concerned about this. What if we don't have it? What if the bots don't have it? https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:12: class LineCoverage(object): This is basically a namedtuple with constants defined inside. Is that necessary? Could we just use a namedtuple? https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:134: def get_status(tr_element): I'm not sure that these need to be local functions. They're a little big, and four of them is a lot. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:180: line = LineCoverage(lineno, source, coverage_status, fractional_coverage) I know I mentioned above about turning LineCoverage into a namedtuple, but another option would be to move all of this <tr> parsing up into the LineCoverage constructor. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:217: full_package_name = package_name + '.' + coverage_file_element.text nit: '%s.%s' % (package_name, coverage_file_element.text)
https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:9: from lxml import html On 2015/07/14 at 16:26:24, jbudorick wrote: > I'm somewhat concerned about this. What if we don't have it? What if the bots don't have it? Was able to replace this with the standard library XML package. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:12: class LineCoverage(object): On 2015/07/14 at 16:26:24, jbudorick wrote: > This is basically a namedtuple with constants defined inside. Is that necessary? Could we just use a namedtuple? Done. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:134: def get_status(tr_element): On 2015/07/14 at 16:26:24, jbudorick wrote: > I'm not sure that these need to be local functions. They're a little big, and four of them is a lot. Done. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:150: # Parse string that contains percent covered: "83% line coverage ,,,". On 2015/07/13 at 16:50:07, mikecase wrote: > Is this supposed to be "83% line coverage ,,," or did you mean "83% line coverage ..." Done. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:180: line = LineCoverage(lineno, source, coverage_status, fractional_coverage) On 2015/07/14 at 16:26:24, jbudorick wrote: > I know I mentioned above about turning LineCoverage into a namedtuple, but another option would be to move all of this <tr> parsing up into the LineCoverage constructor. Chose to change LineCoverage into a namedtuple. I think it makes the code simpler and if possible I'd like to keep the HTML parsing within a single class. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage.... build/android/coverage.py:217: full_package_name = package_name + '.' + coverage_file_element.text On 2015/07/14 at 16:26:24, jbudorick wrote: > nit: '%s.%s' % (package_name, coverage_file_element.text) Done. https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage_... File build/android/coverage_test.py (right): https://codereview.chromium.org/1211243016/diff/20001/build/android/coverage_... build/android/coverage_test.py:287: def MakeElementsWithoutWhitespace(self, html_string): On 2015/07/13 at 16:50:07, mikecase wrote: > This seems a little weird. You could have you Html elements formatted like > > EXAMPLE_HTML = \ > "<html>" + \ > "example" + \ > "</html>" > > so you dont get all the extra whitespace and don't need this function. Done.
It looks like something's ... missing ... here. https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.... build/android/coverage.py:1: #!/usr/bin/python This has #!/usr/bin/python but no main...? How will the bots actually call this? https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.... build/android/coverage.py:130: # Get the line number. nit: line break before thi sline
https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.py File build/android/coverage.py (right): https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.... build/android/coverage.py:1: #!/usr/bin/python On 2015/07/24 at 17:02:59, jbudorick wrote: > This has #!/usr/bin/python but no main...? How will the bots actually call this? I have this implemented in my next CL. https://codereview.chromium.org/1211243016/diff/40001/build/android/coverage.... build/android/coverage.py:130: # Get the line number. On 2015/07/24 at 17:02:59, jbudorick wrote: > nit: line break before thi sline Done.
lgtm
The CQ bit was checked by estevenson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org Link to the patchset: https://codereview.chromium.org/1211243016/#ps60001 (title: "Fixed nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211243016/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211243016/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
nednguyen@google.com changed reviewers: + dtu@chromium.org, nednguyen@google.com
+Dave: This patch is blocked because telemetry_dependencies_unittest complaining about coverage.py being extra dependency . It looks like this is a bug in find_dependencies.py, can you take a look?
On 2015/07/24 20:56:19, nednguyen wrote: > +Dave: This patch is blocked because telemetry_dependencies_unittest complaining > about coverage.py being extra dependency . It looks like this is a bug in > find_dependencies.py, can you take a look? I looked at this. typ (the unit test runner) has imports [1] for a package called "coverage." Because Telemetry adds build/android to its sys.path, the dependency checker interpreted that import as referring to this file. I don't think that code path in typ is used in Telemetry, though, so I've uploaded a patch [2] to the dependency checker to be less aggressive in its static analysis. But you might also want to call this module something else, to avoid future name collisions. There are a bunch of other places in depot_tools and chromium that use pycoverage, and it's checked into the src/third_party. [3] [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/typ/ty... [2] https://codereview.chromium.org/1254173003/ [3] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pycove...
On 2015/07/28 00:20:56, dtu wrote: > On 2015/07/24 20:56:19, nednguyen wrote: > > +Dave: This patch is blocked because telemetry_dependencies_unittest > complaining > > about coverage.py being extra dependency . It looks like this is a bug in > > find_dependencies.py, can you take a look? > > I looked at this. > > typ (the unit test runner) has imports [1] for a package called "coverage." > Because Telemetry adds build/android to its sys.path, the dependency checker > interpreted that import as referring to this file. > > I don't think that code path in typ is used in Telemetry, though, so I've > uploaded a patch [2] to the dependency checker to be less aggressive in its > static analysis. > > But you might also want to call this module something else, to avoid future name > collisions. There are a bunch of other places in depot_tools and chromium that > use pycoverage, and it's checked into the src/third_party. [3] > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/typ/ty... > [2] https://codereview.chromium.org/1254173003/ > [3] > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pycove... We look into this carefully and this is not an easy problem to solve. We only need the dependency_test of telemetry until we move it to catapult, so I would suggest you do the simplest fix here which is renaming your coverage script as Dave had suggested.
Thanks for looking into this, and thanks for the heads up about potential naming conflicts! On Mon, Jul 27, 2015 at 5:20 PM, <dtu@chromium.org> wrote: > On 2015/07/24 20:56:19, nednguyen wrote: > >> +Dave: This patch is blocked because telemetry_dependencies_unittest >> > complaining > >> about coverage.py being extra dependency . It looks like this is a bug in >> find_dependencies.py, can you take a look? >> > > I looked at this. > > typ (the unit test runner) has imports [1] for a package called "coverage." > Because Telemetry adds build/android to its sys.path, the dependency > checker > interpreted that import as referring to this file. > > I don't think that code path in typ is used in Telemetry, though, so I've > uploaded a patch [2] to the dependency checker to be less aggressive in its > static analysis. > > But you might also want to call this module something else, to avoid > future name > collisions. There are a bunch of other places in depot_tools and chromium > that > use pycoverage, and it's checked into the src/third_party. [3] > > > [1] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/typ/ty... > [2] https://codereview.chromium.org/1254173003/ > [3] > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pycove... > > > https://codereview.chromium.org/1211243016/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yeah I've renamed it to emma_coverage_stats.py, should be all good now. Thanks again. On Mon, Jul 27, 2015 at 6:08 PM, <nednguyen@google.com> wrote: > On 2015/07/28 00:20:56, dtu wrote: > >> On 2015/07/24 20:56:19, nednguyen wrote: >> > +Dave: This patch is blocked because telemetry_dependencies_unittest >> complaining >> > about coverage.py being extra dependency . It looks like this is a bug >> in >> > find_dependencies.py, can you take a look? >> > > I looked at this. >> > > typ (the unit test runner) has imports [1] for a package called >> "coverage." >> Because Telemetry adds build/android to its sys.path, the dependency >> checker >> interpreted that import as referring to this file. >> > > I don't think that code path in typ is used in Telemetry, though, so I've >> uploaded a patch [2] to the dependency checker to be less aggressive in >> its >> static analysis. >> > > But you might also want to call this module something else, to avoid >> future >> > name > >> collisions. There are a bunch of other places in depot_tools and chromium >> that >> use pycoverage, and it's checked into the src/third_party. [3] >> > > > [1] >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/typ/ty... > >> [2] https://codereview.chromium.org/1254173003/ >> [3] >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/pycove... > > We look into this carefully and this is not an easy problem to solve. We > only > need the dependency_test of telemetry until we move it to catapult, so I > would > suggest you do the simplest fix here which is renaming your coverage > script as > Dave had suggested. > > https://codereview.chromium.org/1211243016/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Forgot to mention previously -- add your test to the presubmit: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/PRES... https://codereview.chromium.org/1211243016/diff/80001/build/android/emma_cove... File build/android/emma_coverage_stats.py (right): https://codereview.chromium.org/1211243016/diff/80001/build/android/emma_cove... build/android/emma_coverage_stats.py:191: A list of lxml.html.HtmlElements matching the given XPath selector. nit: this is no longer lxml
https://codereview.chromium.org/1211243016/diff/80001/build/android/emma_cove... File build/android/emma_coverage_stats.py (right): https://codereview.chromium.org/1211243016/diff/80001/build/android/emma_cove... build/android/emma_coverage_stats.py:191: A list of lxml.html.HtmlElements matching the given XPath selector. On 2015/07/28 02:45:48, jbudorick wrote: > nit: this is no longer lxml Done.
The CQ bit was checked by estevenson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikecase@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1211243016/#ps100001 (title: "Removed references to lxml and added test to presubmit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211243016/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211243016/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cf902beeaf9f13167abe9d5631ae1d3be9303d92 Cr-Commit-Position: refs/heads/master@{#340833} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
