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

Status: NEW

(Testing :: Code Coverage, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 1

[Greg Mierzwinski [:sparky]]

Attachment: 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/

Comment 2

[Greg Mierzwinski [:sparky]]

Attachment: 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/

Comment 3

[Greg Mierzwinski [:sparky]]

(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.

Comment 4

[Greg Mierzwinski [:sparky]]

Attachment: 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.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 6

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 8

[Greg Mierzwinski [:sparky]]

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?

Comment 9

[Greg Mierzwinski [:sparky]]

Attachment: chrome_map.py

Comment 10

[Greg Mierzwinski [:sparky]]

(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?

Comment 11

[Chris Manchester (limited bugmail, email directly)]

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!

Comment 12

[Chris Manchester (limited bugmail, email directly)]

(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.

Comment 13

[Greg Mierzwinski [:sparky]]

Attachment: chrome-map.json

Comment 14

[Greg Mierzwinski [:sparky]]

(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.

Comment 15

[Greg Mierzwinski [:sparky]]

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 16

[Greg Mierzwinski [:sparky]]

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/

Comment 17

[Greg Mierzwinski [:sparky]]

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/

Comment 18

[Greg Mierzwinski [:sparky]]

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.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 21

[Greg Mierzwinski [:sparky]]

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/

Comment 22

[Greg Mierzwinski [:sparky]]

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/

Comment 23

[Greg Mierzwinski [:sparky]]

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.

Comment 24

[Greg Mierzwinski [:sparky]]

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 25

[Greg Mierzwinski [:sparky]]

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 26

[Greg Mierzwinski [:sparky]]

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/

Comment 27

[Greg Mierzwinski [:sparky]]

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

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

It's removed now.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 29

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 30

[Greg Mierzwinski [:sparky]]

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/

Comment 31

[Greg Mierzwinski [:sparky]]

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/

Comment 32

[Chris Manchester (limited bugmail, email directly)]

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

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

Comment 33

[Chris Manchester (limited bugmail, email directly)]

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

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

Comment 34

[Chris Manchester (limited bugmail, email directly)]

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).

Comment 35

[Greg Mierzwinski [:sparky]]

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.