Closed
Bug 1214885
Opened 9 years ago
Closed 9 years ago
Add a way to map urls to source files for JS code coverage reports
Categories
(Testing :: Code Coverage, defect)
Testing
Code Coverage
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: chmanchester, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(3 files, 3 obsolete files)
The LCOV files emitted to JS_CODE_COVERAGE_OUTPUT_DIR refer to "chrome:" and "resource:" urls as source files, but tools like genhtml need to find source files by path to generate reports. jcranmer mentions a prototype of a tool in https://bugzilla.mozilla.org/show_bug.cgi?id=750347#c1 that sounds ideal for this. Uses of the pre-processor in JS sources might be problematic if we're using files from hg or the srcdir for reference. Even if these uses are rare, I imagine the resulting skew would make reports indecipherable. I'm intending to stand up some amount of support for JS coverage in automation, so it would be great to figure something out that will work with a packaged build. With this in mind, I wonder if it would make sense to dump Debugger.Source instances (either from a test harness or the JS coverage output machinery in SM) in a well known location as they're available, since this will always be available and reflect what's run.
Comment 1•9 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #0) > jcranmer mentions a prototype of a tool in > https://bugzilla.mozilla.org/show_bug.cgi?id=750347#c1 that sounds ideal for > this. It does this, and more! > Uses of the pre-processor in JS sources might be problematic if we're using > files from hg or the srcdir for reference. Not with my tool! ☺ This was actually the second thing I looked at getting working. It's actually not that hard to fix--our preprocess emits //@line directives, and my extract-urls-from-the-build-system tool emits whether or not the file was preprocessed, so it's a fairly easy to do the remapping. What I have so far is the following: 1. A tool that dumps (URI, srcdir, objdir, preprocessed?) tuples for all JS components and modules. 2. A subsequent tool that post-processes the LCOV reports to 2a. Rename the TN: names so that I can assign the results to, say, xpcshell 2b. Remaps URLs to source-dir files 2c. Remaps line numbers of preprocessed files 2d. Drops information that's not possible to relate to source code (e.g., eval, or -e xpcshell options) 2e. Inserts "non-coverage" for unseen JS files, using a really ghetto Reflect.parse-based mechanism. The output meshes quite well with <https://github.com/jcranmer/mozilla-coverage>, which I use to prepare the code coverage web stuff instead of genhtml (whose runtime and output I find, quite frankly, unbearable at this point). I've so far avoided uploading this stuff because there are critical missing pieces: 1. No support for chrome:// URLs yet 2. I need to see how packaged tests screw all of this over 3. I also need to see how I have to hack <https://github.com/jcranmer/m-c-tools-code-coverage> (and possibly more build system hacks) to get this setup to work as simple push-to-try automation.
Comment 2•9 years ago
|
||
This is a new build backend that dumps out the relevant URL map. Since I started the project, glandium completely rebuilt the jar file processor, so I hadn't bothered to look into that stuff yet.
Comment 3•9 years ago
|
||
And this is what is run on the output lcov files.
Updated•9 years ago
|
Attachment #8673991 -
Attachment mime type: text/x-python → text/plain
Comment 4•9 years ago
|
||
So after playing around with trying to support chrome manifests, I've come to realize that my original approach is slightly wrong. Rather than build a single map of (URL, srcdir, objdir, preprocessed) tuples, I'm planning on building two maps: one of (URL, objdir) tuples (primarily directory-based!) and one of (srcdir, objdir, preprocessed) tuples. I think I can support the push-to-try-based code coverage by having the collect-try-results rebuild the objdir sufficiently from test and installer packages (although that will be murder if/when I have to start supporting OS X--dmg files are not trivial).
Reporter | ||
Comment 5•9 years ago
|
||
I'd like to work on getting something landed for this... is comment 2 a reasonable place to start from?
Flags: needinfo?(Pidgeot18)
Comment 6•9 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #5) > I'd like to work on getting something landed for this... is comment 2 a > reasonable place to start from? I seem to recall realizing that there was a major breakage in the script after I did the original upload. <http://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-m-c-new-build/js-code-coverage> is the newest copy of what I have, although I think it's probably still not sufficient.
Flags: needinfo?(Pidgeot18)
Reporter | ||
Comment 7•9 years ago
|
||
Thank you jcranmer. I have a quick version of comment 6 based on install manifests that's working pretty well for a sample directory of browser-chrome mochitests. It doesn't handle line-number remapping yet, which I think because of #included files will require a full parse and rewrite of the lcov output, so I'll file a follow up for that.
Comment 8•9 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #7) > Thank you jcranmer. I have a quick version of comment 6 based on install > manifests that's working pretty well for a sample directory of > browser-chrome mochitests. It doesn't handle line-number remapping yet, > which I think because of #included files will require a full parse and > rewrite of the lcov output, so I'll file a follow up for that. That's what the second attachment in the bug is trying to do (I probably have a newer version of that, though). It also adds "uncoverage" for JS files it knows about (from the above manifest), although those results are insufficiently accurate for tracking code coverage over time.
Reporter | ||
Comment 9•9 years ago
|
||
glandium: I'm going to ask for your review on what I have shortly, because some of this needs build peer review, and you've done a lot of work in this area recently. I'm hoping you might have some suggestions for a simpler way to achieve this. This is working pretty well so far, but is brittle with respect to build system changes (I've had to rework things a bit for the last few days' changes.)
Reporter | ||
Comment 10•9 years ago
|
||
Bug 1214885 - Add a "UrlMap" build backend to write out information useful for resolving chrome urls. The result of running this backend is a json file containing an array with 3 elements: a mapping from url prefixes to objdir directories, a map of override urls, and "extra" intall info (mapping srcfiles to objdir files). The last member may be obsolete once install manifests are generated for more parts of the tree. The files of interest currently missing are those in dist/xpi-stage, which contain mochitest support files.
Attachment #8694445 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 11•9 years ago
|
||
Bug 1214885 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations. This script uses the info generated by the UrlMap backend to replace chrome urls found in lcov files with source-paths. There are several other cases a script might not correspond to a source url, such as "data:" uris, which are handled by omitting those sections in the produced lcov files. The set of files included by a JS source (if it is preprocessed) is also calculated, but not consumed at this time. A more extensive lcov rewriting mechanism will be added in the future to re-construct source information in produced coverage reports.
Attachment #8694446 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26749/diff/1-2/
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8694446 [details] MozReview Request: Bug 1214885 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26751/diff/1-2/
Reporter | ||
Comment 14•9 years ago
|
||
Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.
Updated•9 years ago
|
Attachment #8694445 -
Flags: review?(mh+mozilla)
Comment 15•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium https://reviewboard.mozilla.org/r/26749/#review24415 ::: python/mozbuild/mozbuild/codecoverage/url_map.py:19 (Diff revision 2) > +class SimpleChromeManifestParser(object): A lot of this class could be removed if you used mozpack.chrome.manifest.parse_manifest_line ::: python/mozbuild/mozbuild/codecoverage/url_map.py:79 (Diff revision 2) > + if obj.entry.path.endswith('.manifest'): The right test for this would be isinstance(obj.entry, mozpack.chrome.manifest.Manifest) ::: python/mozbuild/mozbuild/codecoverage/url_map.py:89 (Diff revision 2) > + destdir = '%s/%s/' % (jarmnobj.install_target, info.name) Use mozpath.join, and you need to add jarinfo.base between obj.install_target and info.name if it has a value, and mozpath.normpath the result. ::: python/mozbuild/mozbuild/codecoverage/url_map.py:92 (Diff revision 2) > + if not jarmnobj.install_target.startswith('dist/bin'): > + # We aren't going to be able to find any install manifests > + # for these, because they aren't handled by the faster make > + # back end (although maybe they could be). This is relevant for > + # "chrome://mochikit" for now. > + jarsearchpath = [mozpath.dirname(jarmnobj.path), jarmnobj.objdir] > + for entry in info.entries: > + if entry.source.startswith('/'): > + entry.source = entry.source[1:] > + self._add_install_mapping(entry, [jarmnobj.topsrcdir], > + destdir) > + else: > + self._add_install_mapping(entry, jarsearchpath, > + destdir) I'm not sure what you're trying to achieve here. On one hand you talk about install manifests, but you don't actually use them (and in fact, I don't see a reason why you should). Also, more generally, I'm not sure what this is trying to do. Running it gets me a json file with entries like: "chrome://browser/content/": [ "dist/bin/browser/chrome/browser/content/browser/" ], which arguably doesn't point to a source directory and there's also: "chrome://mochikit/content/": [ "dist/xpi-stage/mochijar/chrome/mochikit/content/" ], so, chrome://mochikit is in there. then, what looks like the list of chrome overrides then, a bunch of things like: "dist/xpi-stage/mochijar/chrome/mochikit/content/ShutdownLeaksCollector.jsm": [ "/home/glandium/gecko/testing/mochitest/ShutdownLeaksCollector.jsm", false ], for dist/xpi-stage/ directories only. (and then a list of .manifest files) Could you clarify? ::: python/mozbuild/mozbuild/config_status.py:112 (Diff revision 2) > + 'UrlMap'], ChromeMap would be a better name, IMHO. ::: python/mozbuild/mozbuild/mach_commands.py:583 (Diff revision 2) > - 'VisualStudio', 'FasterMake', 'CompileDB'], > + 'VisualStudio', 'FasterMake', 'CompileDB', 'UrlMap'], You need to add one to build/autoconf/config.status.m4 as well.
Reporter | ||
Comment 16•9 years ago
|
||
https://reviewboard.mozilla.org/r/26749/#review24415 > A lot of this class could be removed if you used mozpack.chrome.manifest.parse_manifest_line Thanks, will do. > I'm not sure what you're trying to achieve here. On one hand you talk about install manifests, but you don't actually use them (and in fact, I don't see a reason why you should). > > Also, more generally, I'm not sure what this is trying to do. Running it gets me a json file with entries like: > > "chrome://browser/content/": [ > "dist/bin/browser/chrome/browser/content/browser/" > ], > > which arguably doesn't point to a source directory > > and there's also: > > "chrome://mochikit/content/": [ > "dist/xpi-stage/mochijar/chrome/mochikit/content/" > ], > > so, chrome://mochikit is in there. > > then, what looks like the list of chrome overrides > > then, a bunch of things like: > > "dist/xpi-stage/mochijar/chrome/mochikit/content/ShutdownLeaksCollector.jsm": [ > "/home/glandium/gecko/testing/mochitest/ShutdownLeaksCollector.jsm", > false > ], > > for dist/xpi-stage/ directories only. > > (and then a list of .manifest files) > > Could you clarify? Thanks for the feedback (and sorry for the lack of context). The rest of this is in the second commit in the series, but briefly, the first part of the output is used to find/replace on urls to find the objdir file, which is then found in an install manifest to get ther original source file. The second element is the "final" output for those files that wont be found in install manifests. I didn't see a way to get this for every file at this stage because of the way "resource://devtools" works (the paths are put in FINAL_TARGET_FILES instead of jar.mn, so I don't have the full path). The last element is extra manifests parsed by the processor and added to the prefixes to replace (although this could just be done here).
Reporter | ||
Comment 17•9 years ago
|
||
https://reviewboard.mozilla.org/r/26749/#review24415 > Thanks for the feedback (and sorry for the lack of context). The rest of this is in the second commit in the series, but briefly, the first part of the output is used to find/replace on urls to find the objdir file, which is then found in an install manifest to get ther original source file. The second element is the "final" output for those files that wont be found in install manifests. I didn't see a way to get this for every file at this stage because of the way "resource://devtools" works (the paths are put in FINAL_TARGET_FILES instead of jar.mn, so I don't have the full path). The last element is extra manifests parsed by the processor and added to the prefixes to replace (although this could just be done here). Sorry, the the _third_ element is the "final" output for files that aren't in install manifests. The second looks like and is a list of chrome overrides.
Reporter | ||
Comment 18•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26749/diff/2-3/
Attachment #8694445 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 19•9 years ago
|
||
Comment on attachment 8694446 [details] MozReview Request: Bug 1214885 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26751/diff/2-3/
Reporter | ||
Comment 20•9 years ago
|
||
Comment on attachment 8694463 [details] MozReview Request: Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26753/diff/1-2/
Reporter | ||
Comment 21•9 years ago
|
||
I just chatted with :glandium about this, and he has patches in progress that will greatly simplify or render this obsolete by centralizing the way the build system handles chrome (and exposing this at least to the faster make backend). I'm going to shelve these patches for now pending those developments. An advantage to this is that this will generally not break once the we have developers using faster make on a regular basis.
Reporter | ||
Comment 22•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26749/diff/3-4/
Attachment #8694445 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 23•9 years ago
|
||
Comment on attachment 8694446 [details] MozReview Request: Bug 1214885 - Add a script to re-write lcov files, replacing chrome urls with sourcefile locations. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26751/diff/3-4/
Attachment #8694446 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 24•9 years ago
|
||
Comment on attachment 8694463 [details] MozReview Request: Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26753/diff/2-3/
Reporter | ||
Comment 25•9 years ago
|
||
Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.
Reporter | ||
Comment 26•9 years ago
|
||
Setting need info re: comment 21 so this doesn't fall through the cracks.
Flags: needinfo?(mh+mozilla)
Comment 27•9 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #26) > Setting need info re: comment 21 so this doesn't fall through the cracks. That is now all in bug 1235021, waiting for review.
Flags: needinfo?(mh+mozilla)
Reporter | ||
Comment 28•9 years ago
|
||
I updated the patches based on bug 1235021, and I'll request review on the first shortly. The result is a dramatic simplification.
Reporter | ||
Comment 29•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26749/diff/3-4/
Attachment #8694445 -
Attachment description: MozReview Request: Bug 1214885 - Add a "UrlMap" build backend to write out information useful for resolving chrome urls. → MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls.
Reporter | ||
Updated•9 years ago
|
Attachment #8694446 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8694463 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8697329 -
Attachment is obsolete: true
Reporter | ||
Comment 30•9 years ago
|
||
(In reply to Chris Manchester [:chmanchester] from comment #28) > I updated the patches based on bug 1235021, and I'll request review on the > first shortly. The result is a dramatic simplification. glandium isn't accepting review requests right now, but this can wait until next week.
Reporter | ||
Updated•9 years ago
|
Attachment #8694445 -
Flags: review?(mh+mozilla)
Comment 31•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium https://reviewboard.mozilla.org/r/26749/#review27821 ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:44 (Diff revision 4) > + entry.path) You have three conditions that end up doing mostly the same thing, so I'd suggest avoiding code duplication. Like: if isinstance(entry, (ManifestChrome, ManifestResource)): if isinstance(entry, ManifestResource): dest = entry.target if not urlparse.urlparse(dest).scheme: dest = mozpath.join(entry.base, dest) else: dest = entry.path self.add_chrome_map(...) (Also note that I don't see a specific reason to exclude skin entries, thus ManifestChrome) And since there's only one call site for add_chrome_map, you could inline it. With that being said, I do realize that this code, as well as several other things in the packager code could benefit from having two separate classes for ManifestResource, one that inherits from ManifestEntryWithRelPath, and one matching the current one, depending on whether the target is relative path or an url. If that feels like something you'd like to take care of, be my guest :) Relatedly, there's this completely weird entry in webapprt/locales/jar.mn that is not handled properly: locale branding @AB_CD@ resource://webappbranding/ ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:62 (Diff revision 4) > + target = target.replace('file://', '') > + # The mochitest harness produces some file:// uris that need > + # to be fixed. > + while target.startswith('//'): > + target = target.replace('//', '/') > + target = target.replace('/c:/', 'c:/') Shouldn't urlparse.urlparse(target).path give you what you want if .scheme is 'file'? Plus, if I just remove this chunk, the chrome-info.json is unchanged, but I'm on linux and I may have been too smart in the build steps I ran before this. ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:68 (Diff revision 4) > + self.chrome_mapping[base_uri].add(target) Some paths end with a /, some not, and one has a /./, you may want to mozpath.normpath() ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:77 (Diff revision 4) > + self._exts = {'.js', '.jsm', '.html', '.xul', '.xml', '.xhtml'} not sure why you'd want to restrict this. I could see .css being interesting, and possibly other types. Also, once you get the nicety of having this mapping for code files, you get to want it for images and such... ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:110 (Diff revision 4) > + outputfile = os.path.join(self.environment.topobjdir, 'chrome-info.json') It seems to me chrome-map.json would be more obvious to find considering the name of the backend :)
Attachment #8694445 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 32•9 years ago
|
||
https://reviewboard.mozilla.org/r/26749/#review27821 > You have three conditions that end up doing mostly the same thing, so I'd suggest avoiding code duplication. > > Like: > if isinstance(entry, (ManifestChrome, ManifestResource)): > if isinstance(entry, ManifestResource): > dest = entry.target > if not urlparse.urlparse(dest).scheme: > dest = mozpath.join(entry.base, dest) > else: > dest = entry.path > > self.add_chrome_map(...) > > (Also note that I don't see a specific reason to exclude skin entries, thus ManifestChrome) > > And since there's only one call site for add_chrome_map, you could inline it. > > With that being said, I do realize that this code, as well as several other things in the packager code could benefit from having two separate classes for ManifestResource, one that inherits from ManifestEntryWithRelPath, and one matching the current one, depending on whether the target is relative path or an url. If that feels like something you'd like to take care of, be my guest :) > > Relatedly, there's this completely weird entry in webapprt/locales/jar.mn that is not handled properly: > locale branding @AB_CD@ resource://webappbranding/ I think I noticed that weird entry... it doesn't seem to be following the rules, or referenced anywhere, and we may be getting rid of webapprt altogether, so I'm ok with ignoring it in the absence on an obvious fix. I'll take a look at ManifestResource. > Shouldn't urlparse.urlparse(target).path give you what you want if .scheme is 'file'? Plus, if I just remove this chunk, the chrome-info.json is unchanged, but I'm on linux and I may have been too smart in the build steps I ran before this. This was a problem feeding in manifests produced by the mochitest harness, I'll just fix it there or elsewhere.
Reporter | ||
Comment 33•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26749/diff/4-5/
Attachment #8694445 -
Attachment description: MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. → MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium
Attachment #8694445 -
Flags: review?(mh+mozilla)
Updated•9 years ago
|
Attachment #8694445 -
Flags: review?(mh+mozilla) → review+
Comment 34•9 years ago
|
||
Comment on attachment 8694445 [details] MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium https://reviewboard.mozilla.org/r/26749/#review28097 ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:45 (Diff revisions 4 - 5) > + url = urlparse.urlparse(entry.target) urlparse(dest) ::: python/mozbuild/mozbuild/codecoverage/chrome_map.py:48 (Diff revisions 4 - 5) > + if url.scheme == 'file': elif, but I'm still curious what is adding file:// urls for you, because I don't see them in my local build.
Reporter | ||
Comment 35•9 years ago
|
||
https://reviewboard.mozilla.org/r/26749/#review28097 > elif, but I'm still curious what is adding file:// urls for you, because I don't see them in my local build. I was using this class to parse chrome manifests generated by the mochitest harness -- https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/testing/mochitest/runtests.py#882
Comment 37•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a58563fd4e6c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•