Open
Bug 1255179
Opened 9 years ago
Updated 2 years ago
Use the chrome map data to re-write jsdcov coverage output
Categories
(Testing :: Code Coverage, defect)
Testing
Code Coverage
Tracking
(Not tracked)
NEW
People
(Reporter: chmanchester, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
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•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
(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•9 years ago
|
||
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•9 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•9 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•9 years ago
|
Attachment #8734815 -
Flags: review?(cmanchester)
Reporter | ||
Comment 7•9 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.
Comment 8•9 years ago
|
||
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•9 years ago
|
||
Comment 10•9 years ago
|
||
(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•9 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•9 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.
Comment 13•9 years ago
|
||
Attachment #8736773 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
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 17•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
Attachment #8734816 -
Flags: review?(cmanchester)
Reporter | ||
Comment 19•9 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•9 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 21•9 years ago
|
||
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 22•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
https://reviewboard.mozilla.org/r/42491/#review42483
> I'll have this fixed in the next revision.
It's removed now.
Reporter | ||
Comment 28•9 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•9 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 30•9 years ago
|
||
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 31•9 years ago
|
||
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•8 years ago
|
Attachment #8734816 -
Flags: review?(cmanchester)
Reporter | ||
Comment 32•8 years ago
|
||
Comment on attachment 8734816 [details]
MozReview Request: Bug 1255179 - Url Finder adition.
https://reviewboard.mozilla.org/r/42493/#review54866
Reporter | ||
Comment 33•8 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•8 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).
Updated•6 years ago
|
Summary: Use the chrome map data to re-write json coverage output → Use the chrome map data to re-write jsdcov coverage output
Comment 35•6 years ago
|
||
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.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•