Use the chrome map data to re-write jsdcov coverage output

NEW
Unassigned

Status

3 years ago
5 months ago

People

(Reporter: chmanchester, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

This is related to bug 1224691, but for the JSON output we're getting from browser chrome tests. Extracting https://reviewboard.mozilla.org/r/32685/diff/1#index_header would be a good place to start.
Created attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

This contains the json file rewriter which is responsible for mapping URL's found in the "sourceFile" field of code coverage json artifacts to local system paths.

Review commit: https://reviewboard.mozilla.org/r/42491/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42491/
Attachment #8734815 - Flags: review?(cmanchester)
Created attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

This contains the url finder class which is responsible for mapping the URL's to local source file locations. It was taken from lcov_rewriter.py.

Review commit: https://reviewboard.mozilla.org/r/42493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42493/
Attachment #8734816 - Flags: review?(cmanchester)
(In reply to Greg Mierzwinski from comment #1)
> Created attachment 8734815 [details]
> MozReview Request: Bug 1255179 - Json Rewriter addition.
> 
> This contains the json file rewriter which is responsible for mapping URL's
> found in the "sourceFile" field of code coverage json artifacts to local
> system paths.
> 
> Review commit: https://reviewboard.mozilla.org/r/42491/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/42491/

I've removed the whitespace so it won't be there in the next revision.
Created attachment 8735955 [details]
jscov_1455672032084.json

Here is the output of running the "json_rewriter.py" on "jscov_1455672032084.json" (attached) : https://pastebin.mozilla.org/8865498

There are 5 files which were not mapped for some reason. I am thinking that XStringBundle shouldn't map to anything because it has no resource:/ or chrome:/ as a prefix, so it seems to me as if at least that one is correctly identified as an error.
(Reporter)

Comment 5

3 years ago
We should be able to map all of those, I'm not sure what's going wrong there. Can you upload the chrome map file as well?
(Reporter)

Comment 6

3 years ago
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

https://reviewboard.mozilla.org/r/42493/#review39919

::: python/mozbuild/mozbuild/codecoverage/url_finder.py:63
(Diff revision 1)
> +            self._respath = mozpath.join('dist',
> +                                         buildconfig.substs['MOZ_MACBUNDLE_NAME'],
> +                                         'Contents',
> +                                         'Resources')
> +
> +        #self._populate_chrome(extra_chrome_manifests)

I was looking at this locally, and I think this line is needed to map some of our files related to tests. We should look at getting this working.
Attachment #8734816 - Flags: review?(cmanchester)
(Reporter)

Updated

3 years ago
Attachment #8734815 - Flags: review?(cmanchester)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

https://reviewboard.mozilla.org/r/42491/#review39923

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:29
(Diff revision 1)
> +    # Class for partial parses of JSON format and rewriting to resolve urls
> +    # and preprocessed file lines.
> +    def __init__(self, appdir, gredir, extra_chrome_manifests):
> +        self.topobjdir = buildconfig.topobjdir
> +        self.url_finder = UrlFinder(appdir, gredir, extra_chrome_manifests)
> +        self._line_comment_re = re.compile('//@line \d+ "(.+)"$')

This isn't used anymore.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:36
(Diff revision 1)
> +            for line in fh:
> +                if "sourceFile" in line:
> +                    url = line[19:].rstrip()
> +                    url = url[:-2]
> +                    try:
> +                        res = self.url_finder.rewrite_url(url)
> +                        if res is None:

Let's try to do this by parsing the json structure and re-writing fields, rather than line by line. You can use the Python "json" module for this.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:57
(Diff revision 1)
> +    parser = ArgumentParser(description="Given a set of gcov .info files produced "
> +                            "by spidermonkey's code coverage, re-maps file urls "
> +                            "back to source files and lines in preprocessed files "
> +                            "back to their original locations.")

Let's update this comment.
https://reviewboard.mozilla.org/r/42493/#review39919

> I was looking at this locally, and I think this line is needed to map some of our files related to tests. We should look at getting this working.

That would mean that those missing files may get mapped after we have this working? Also, is it commented out because it is not working, or is there another reason?
(In reply to Chris Manchester (:chmanchester) from comment #5)
> We should be able to map all of those, I'm not sure what's going wrong
> there. Can you upload the chrome map file as well?

Here's chrome_map.py, is that the file you're asking for?
(Reporter)

Comment 11

3 years ago
https://reviewboard.mozilla.org/r/42493/#review39919

> That would mean that those missing files may get mapped after we have this working? Also, is it commented out because it is not working, or is there another reason?

Yeah, at least some of them are mapped by that file. It looks like you commented it out at some point, so I'm not sure!
(Reporter)

Comment 12

3 years ago
(In reply to Greg Mierzwinski from comment #10)
> (In reply to Chris Manchester (:chmanchester) from comment #5)
> > We should be able to map all of those, I'm not sure what's going wrong
> > there. Can you upload the chrome map file as well?
> 
> Here's chrome_map.py, is that the file you're asking for?

I meant the chrome-map.json produced by the build, but I think the main issue here is in comment 8.
Created attachment 8736774 [details]
chrome-map.json
Attachment #8736773 - Attachment is obsolete: true
(In reply to Chris Manchester (:chmanchester) from comment #12)
> (In reply to Greg Mierzwinski from comment #10)
> > (In reply to Chris Manchester (:chmanchester) from comment #5)
> > > We should be able to map all of those, I'm not sure what's going wrong
> > > there. Can you upload the chrome map file as well?
> > 
> > Here's chrome_map.py, is that the file you're asking for?
> 
> I meant the chrome-map.json produced by the build, but I think the main
> issue here is in comment 8.

Ok, I've added it if you're still interested in looking at it. I'll look at the problem in comment 8 and let you know how it goes.
https://reviewboard.mozilla.org/r/42493/#review39919

> Yeah, at least some of them are mapped by that file. It looks like you commented it out at some point, so I'm not sure!

Oh whoops! I may have commented it out when I was having problems mapping files and forgot about it. I'll fix this and let you know if any new problems come up (or if any problems are fixed).
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42491/diff/1-2/
Attachment #8734815 - Flags: review?(cmanchester)
Attachment #8734816 - Flags: review?(cmanchester)
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42493/diff/1-2/
https://reviewboard.mozilla.org/r/42493/#review39919

> Oh whoops! I may have commented it out when I was having problems mapping files and forgot about it. I'll fix this and let you know if any new problems come up (or if any problems are fixed).

After adding the flags that cause errors for 'remoteenabled' and 'remoterequired' and then removing the comment, there are still certain files not being mapped.
(Reporter)

Updated

3 years ago
Attachment #8734816 - Flags: review?(cmanchester)
(Reporter)

Comment 19

3 years ago
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

https://reviewboard.mozilla.org/r/42493/#review42479

::: python/mozbuild/mozbuild/codecoverage/url_finder.py:21
(Diff revision 2)
> +ManifestContent.allowed_flags += ['remoteenabled', 'remoterequired']
> +Flags.FLAGS.update({
> +        'remoteenabled': Flag,
> +        'remoterequired': Flag
> +})

We can remove this if we're not attempting to map the testing files we talked about.

::: python/mozbuild/mozbuild/codecoverage/url_finder.py:37
(Diff revision 2)
> +
> +class UrlFinder(object):
> +    # Given a "chrome://" or "resource://" url, uses data from the UrlMapBackend
> +    # and install manifests to find a path to the source file and the corresponding
> +    # (potentially pre-processed) file in the objdir.
> +    def __init__(self, appdir, gredir, extra_chrome_manifests):

Let's leave out these bits related to extra_chrome_manifests, but leave a TODO comment about getting this working.
(Reporter)

Comment 20

3 years ago
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

https://reviewboard.mozilla.org/r/42491/#review42483

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:12
(Diff revision 2)
> +from mozpack.copier import FileRegistry
> +from mozpack.files import PreprocessedFile
> +from mozpack.manifests import InstallManifest
> +from mozpack.chrome.manifest import parse_manifest

Some of these imports are unused. Have you ever used pyflakes when working on Python? It's a nice linter that will warn about things like this.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:25
(Diff revision 2)
> +    # Class for partial parses of JSON format and rewriting to resolve urls
> +    # and preprocessed file lines.

This comment could use an update.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:33
(Diff revision 2)
> +        new_path = in_path[:-5]
> +        out_path = new_path + output_suffix

I think we'll want the output files to still be .json... can we do something like <file>.json -> <file>.out.json ?

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:37
(Diff revision 2)
> +        in_path = os.path.abspath(in_path)
> +        new_path = in_path[:-5]
> +        out_path = new_path + output_suffix
> +        with open(in_path, 'r') as fh, open(out_path, 'w') as out_fh:
> +            data = json.load(fh, object_pairs_hook=OrderedDict)
> +            fh.close()

No need to close here, the context manager will take care of that at the end of the with block.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:39
(Diff revision 2)
> +                for field in data[key]:
> +                    if "sourceFile" in field:
> +                        url = data[key][field]

Our top level is an array, right? I think this loop should read something like:

```
for record in data:
    name = record.get('sourceFile')
    if name:
        ...
```

etc.
Attachment #8734815 - Flags: review?(cmanchester)
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42491/diff/2-3/
Attachment #8734815 - Flags: review?(cmanchester)
Attachment #8734816 - Flags: review?(cmanchester)
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42493/diff/2-3/
https://reviewboard.mozilla.org/r/42491/#review42483

> No need to close here, the context manager will take care of that at the end of the with block.

I'll have this fixed in the next revision.
https://reviewboard.mozilla.org/r/42493/#review42479

> Let's leave out these bits related to extra_chrome_manifests, but leave a TODO comment about getting this working.

I removed what I could and also made the needed changes to the json_rewriter which passes the file to the UrlFinder.
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42491/diff/3-4/
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42493/diff/3-4/
https://reviewboard.mozilla.org/r/42491/#review42483

> I'll have this fixed in the next revision.

It's removed now.
(Reporter)

Comment 28

3 years ago
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

https://reviewboard.mozilla.org/r/42491/#review43881

Just some small things to address. Thanks!

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:25
(Diff revisions 2 - 4)
>          new_path = in_path[:-5]
>          out_path = new_path + output_suffix

Nit: we can use os.path.splitext here.

::: python/mozbuild/mozbuild/codecoverage/json_rewriter.py:42
(Diff revisions 2 - 4)
> -                        except UrlFinderError as e:
> +                    except UrlFinderError as e:
> -                            print("Error: %s.\nCouldn't find source info for %s" % (e, url))
> +                        print("Error: %s.\nCouldn't find source info for %s" % (e, url))
> -                            continue
> +                        continue
> -                        src_file, objdir_file, _ = res
> +                    src_file, objdir_file, _ = res
> -                        assert os.path.isfile(src_file), "Couldn't find mapped source file at %s!" % src_file
> +                    assert os.path.isfile(src_file), "Couldn't find mapped source file at %s!" % src_file
> -                        data[key][field] = src_file
> +                    data[index]['sourceFile'] = src_file

I think `data[index]` is `record` in this loop, can we use `record` in this assignment?
Attachment #8734815 - Flags: review?(cmanchester)
(Reporter)

Comment 29

3 years ago
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

https://reviewboard.mozilla.org/r/42493/#review43885

::: python/mozbuild/mozbuild/codecoverage/url_finder.py:80
(Diff revision 4)
> +    def _populate_chrome(self, manifests):
> +        handler = ChromeManifestHandler()
> +        for m in manifests:
> +            path = os.path.abspath(m)
> +            for e in parse_manifest(None, path):
> +                handler.handle_manifest_entry(e)
> +        self._url_overrides.update(handler.overrides)
> +        self._url_prefixes.update(handler.chrome_mapping)

This method can be removed now that it has no callers.
Attachment #8734816 - Flags: review?(cmanchester)
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42491/diff/4-5/
Attachment #8734815 - Flags: review?(cmanchester)
Attachment #8734816 - Flags: review?(cmanchester)
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42493/diff/4-5/
(Reporter)

Updated

3 years ago
Attachment #8734816 - Flags: review?(cmanchester)
(Reporter)

Comment 33

3 years ago
Comment on attachment 8734815 [details]
MozReview Request: Bug 1255179 - Json Rewriter addition.

https://reviewboard.mozilla.org/r/42491/#review54868
Attachment #8734815 - Flags: review?(cmanchester)
(Reporter)

Comment 34

3 years ago
Greg, I was thinking about this some more, and I think we'll need something that runs as a standalone script to make this useful for out use cases.

What we have here is good for developing locally, because we'll have a build environment, but to make this work elsewhere we'll want to be able to do this processing without a build environment available.

I think this can be achieved by uploading the chrome map file during the build step, and using the mozbuild package from pypi for the manifest parsing (everything involving mozpack).
Summary: Use the chrome map data to re-write json coverage output → Use the chrome map data to re-write jsdcov coverage output
The linux64-jsdcov build has been disabled, and no longer runs in taskcluster, see bug 1496791. This bug will remain open as it relates to local development as well.
See Also: → bug 1496791
You need to log in before you can comment on or make changes to this bug.