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)

Product:
Testing
Component:
Code Coverage
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: chmanchester, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

Prototype URL map dumper
(deleted), patch
Details | Diff | Splinter Review
Prototype post-processing tool
(deleted), text/plain
Details
MozReview Request: Bug 1214885 - Add a "ChromeUrl" build backend to write out information useful for resolving chrome urls. r=glandium
(deleted), text/x-review-board-request
glandium
: review+
Details 
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.
(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.
Attached patch Prototype URL map dumper (deleted) — — Splinter Review 
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.
Attached file Prototype post-processing tool (deleted) — 
And this is what is run on the output lcov files.
Attachment #8673991 - Attachment mime type: text/x-python → text/plain
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).
I'd like to work on getting something landed for this... is comment 2 a reasonable place to start from?
Flags: needinfo?(Pidgeot18)
(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)
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.
(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.
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.)
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)
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)
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/
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/
Bug 1224691 - Parse lcov files and rewrite them based on preprocessor info.
Attachment #8694445 - Flags: review?(mh+mozilla)
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.
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).
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.
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)
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/
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/
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.
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)
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)
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/
Bug 1229598 - Add a mode to browser-chrome tests to summarize per-test code coverage.
Setting need info re: comment 21 so this doesn't fall through the cracks.
Flags: needinfo?(mh+mozilla)
Depends on: 1235021
(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)
I updated the patches based on bug 1235021, and I'll request review on the first shortly. The result is a dramatic simplification.
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.
Attachment #8694446 - Attachment is obsolete: true
Attachment #8694463 - Attachment is obsolete: true
Attachment #8697329 - Attachment is obsolete: true
(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.
Attachment #8694445 - Flags: review?(mh+mozilla)
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)
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.
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)
Attachment #8694445 - Flags: review?(mh+mozilla) → review+
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.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58563fd4e6c
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.

Attachment

General

Creator:
Created:
Updated:
Size: