Rewrite jar: paths in JS coverage lcov output

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: marco, Assigned: marco)

Tracking

(Blocks: 1 bug)

Version 3
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

a year ago
Since we're generating the chrome-map.json file using a normal build and we're running the tests using a packaged build, there are some paths that we currently can't map.

Examples (LCOV <-> chrome-map.json)
jar:file://OBJ_DIR/application/firefox/omni.ja!/components <-> dist/bin/components
jar:file://OBJ_DIR/application/firefox/browser/omni.ja!/components <-> dist/bin/browser/components
jar:file:///tmp/tmpMdo5gV.mozrunner/extensions/workerbootstrap-test@mozilla.org.xpi!/bootstrap.js <-> dist/xpi-stage/workerbootstrap/bootstrap.js

/home/worker/workspace/build/tests/bin/components/httpd.js <-> dist/bin/components/httpd.js (or _tests/modules/httpd.js)


The first two can actually be simply mapped in lcov-rewriter.py, by simply replacing "jar:file://OBJ_DIR/application/firefox/omni.ja!/components" with "dist/bin/components" and "jar:file://OBJ_DIR/application/firefox/browser/omni.ja!/components" with "dist/bin/browser/components", but there could be a cleaner way to do this.
(Assignee)

Updated

a year ago
Blocks: 1189360
I looked in to this this afternoon, and the omnijar format appears to just apply some heuristics and create an omnijar in-place where needed without otherwise messing with paths. So the replacement in comment 0 should actually work fine for desktop (at least on Linux), and the slightly more robust way would be to determine what part of the path is under "firefox" (or MOZ_APP_NAME), strip out omni.ja!/, and treat the result as being under dist/bin, which we should then find in the chrome map.
(In reply to Chris Manchester (:chmanchester) from comment #1)
> ... strip out omni.ja!/, ...

This can be based on OMNIJAR_NAME.
(Assignee)

Updated

a year ago
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Created attachment 8879499 [details] [diff] [review]
Patch
Attachment #8879499 - Flags: review?(cmanchester)
Comment on attachment 8879499 [details] [diff] [review]
Patch

Review of attachment 8879499 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this! A few small things I'd like to see before landing.

::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py
@@ +602,5 @@
> +                elif '.xpi!' in url:
> +                    # e.g. file:///home/worker/workspace/build/application/firefox/browser/features/e10srollout@mozilla.org.xpi!/bootstrap.js
> +                    parts = url_obj.path.split('.xpi!', 1)
> +                else:
> +                    return url_obj.path, None, False

Which case is this? Shouldn't we check this path actually exists?

@@ +605,5 @@
> +                else:
> +                    return url_obj.path, None, False
> +
> +                dir_parts = parts[0].rsplit(app_name + '/', 1)
> +                url = os.path.join(self.topobjdir, 'dist/bin', dir_parts[1].lstrip('/'), parts[1].lstrip('/'))

nit: "os.path.join(self.topobjdir, 'dist', 'bin', ..."

@@ +606,5 @@
> +                    return url_obj.path, None, False
> +
> +                dir_parts = parts[0].rsplit(app_name + '/', 1)
> +                url = os.path.join(self.topobjdir, 'dist/bin', dir_parts[1].lstrip('/'), parts[1].lstrip('/'))
> +            elif 'mozrunner' in url and '.xpi!' in url:

Do we need to check for 'mozrunner' here?

@@ +611,5 @@
> +                # e.g. file:///tmp/tmpMdo5gV.mozrunner/extensions/workerbootstrap-test@mozilla.org.xpi!/bootstrap.js
> +                parts = url_obj.path.split('.xpi!', 1)
> +                addon_name = os.path.basename(parts[0])
> +                if '-test@mozilla.org' in addon_name:
> +                    addon_name = addon_name[:-len('-test@mozilla.org')]

We should note that this is pretty brittle: nothing's enforcing the relationship between XPI_NAME in these directories and this path right now. Not sure we can do better.

::: python/mozbuild/mozbuild/test/codecoverage/test_lcov_rewrite.py
@@ +249,5 @@
> +        app_name = buildconfig.substs.get('MOZ_APP_NAME')
> +        omnijar_name = buildconfig.substs.get('OMNIJAR_NAME')
> +
> +        paths = [
> +            ('jar:file:///home/worker/workspace/build/application/' + app_name + '/' + omnijar_name + '!/components/MainProcessSingleton.js', 'dist/bin/components/MainProcessSingleton.js'),

Can we add a test for paths in the omnijar under "firefox/browser/omni.ja!"?
Attachment #8879499 - Flags: review?(cmanchester)
(Assignee)

Comment 5

a year ago
Created attachment 8880473 [details] [diff] [review]
Patch

(In reply to Chris Manchester (:chmanchester) from comment #4)
> Comment on attachment 8879499 [details] [diff] [review]
> Patch
> 
> Review of attachment 8879499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for tackling this! A few small things I'd like to see before landing.
> 
> ::: python/mozbuild/mozbuild/codecoverage/lcov_rewriter.py
> @@ +602,5 @@
> > +                elif '.xpi!' in url:
> > +                    # e.g. file:///home/worker/workspace/build/application/firefox/browser/features/e10srollout@mozilla.org.xpi!/bootstrap.js
> > +                    parts = url_obj.path.split('.xpi!', 1)
> > +                else:
> > +                    return url_obj.path, None, False
> 
> Which case is this? Shouldn't we check this path actually exists?

This is an unexpected case, I'm not returning None so that the caller prints a warning (so if we find such a case in the future we have a chance to notice it and implement it).
I've added a comment in the new version of the patch.


> 
> @@ +606,5 @@
> > +                    return url_obj.path, None, False
> > +
> > +                dir_parts = parts[0].rsplit(app_name + '/', 1)
> > +                url = os.path.join(self.topobjdir, 'dist/bin', dir_parts[1].lstrip('/'), parts[1].lstrip('/'))
> > +            elif 'mozrunner' in url and '.xpi!' in url:
> 
> Do we need to check for 'mozrunner' here?

I don't think so, it was maybe too conservative, I've removed the check.
Attachment #8879499 - Attachment is obsolete: true
Attachment #8880473 - Flags: review?(cmanchester)
(Assignee)

Comment 6

a year ago
Created attachment 8880548 [details] [diff] [review]
Interdiff

Tests were failing, so I've had to make this change creating a dummy chrome-map.json file.
(Assignee)

Comment 7

a year ago
Created attachment 8880550 [details] [diff] [review]
Patch
Attachment #8880473 - Attachment is obsolete: true
Attachment #8880548 - Attachment is obsolete: true
Attachment #8880550 - Flags: review?(cmanchester)
Attachment #8880473 - Flags: review?(cmanchester)
Comment on attachment 8880550 [details] [diff] [review]
Patch

Review of attachment 8880550 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you for updating this!
Attachment #8880550 - Flags: review?(cmanchester) → review+

Comment 10

a year ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a44b036e41
Rewrite paths from JAR files. r=chmanchester

Comment 11

a year ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/671e4c9ac8a1
Backed out changeset b7a44b036e41 for failing test_lcov_rewrite.py on Windows
(Assignee)

Comment 13

a year ago
The try build on Windows was successful :/
(Assignee)

Comment 14

a year ago
I've made some changes that I think should fix the problem (the try build is green again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7046175fd0227acf2f7bd806c44010bd91164eeb).

Comment 15

a year ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ececd1d2c875
Rewrite paths from JAR files. r=chmanchester

Comment 16

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57eeeb588417
Backed out changeset ececd1d2c875 for bustage
(In reply to Marco Castelluccio [:marco] from comment #14)
> I've made some changes that I think should fix the problem (the try build is
> green again:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7046175fd0227acf2f7bd806c44010bd91164eeb).

Maybe we should be using `mozpath` instead of os.path in lcov_rewriter.py, it normalizes paths it handles to be unix style.
(Assignee)

Comment 18

a year ago
I'll try to reland with mozpath instead of os.path, the try build is green again (https://treeherder.mozilla.org/#/jobs?repo=try&revision=87631594b999763cd73377494b9c90b4582619e8).

Comment 19

a year ago
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1334d017fc5b
Rewrite paths from JAR files. r=chmanchester

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1334d017fc5b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.