add support to the xpcshell harness for code coverage collection

RESOLVED FIXED in Firefox 50

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jmaher, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
as we are having the ability to collect code coverage more easily, lets expand our hooks into test harnesses.

we should instrument xpcshell so that if given a commandline option it would import coverageutils.jsm and store off coverage data properly.
Created attachment 8761211 [details]
Bug 1278649 - Add command line argument to collect code coverage.

This adds the flag '--jscov-dir-prefix' to the list of available command line arguments for xpcshell testing.

Review commit: https://reviewboard.mozilla.org/r/58504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58504/
Attachment #8761211 - Flags: review?(cmanchester)
Attachment #8761212 - Flags: review?(cmanchester)
Created attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

This adds the ability to use the command line flag '--jscov-dir-prefix' to collect code coverage and output it into the specified directory.

Review commit: https://reviewboard.mozilla.org/r/58506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58506/
Comment on attachment 8761211 [details]
Bug 1278649 - Add command line argument to collect code coverage.

https://reviewboard.mozilla.org/r/58504/#review56770

Looks good, although I think this might be better folded into the next patch...
Attachment #8761211 - Flags: review?(cmanchester) → review+
Attachment #8761212 - Flags: review?(cmanchester)
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

https://reviewboard.mozilla.org/r/58506/#review56774

Just a few nits here, thanks for the patch. Two things to resolve before we get this landed: the previous patch should be folded into this one, and have we gotten this going on try yet? It would be nice to have the mozharness bits for this landed in this bug, or a follow up on file to get it going there.

::: testing/xpcshell/head.js:522
(Diff revision 1)
> +  var coverageCollector = null;
> +  if (typeof _JSCOV_DIR === 'string') {
> +    var jscov_output_dir = _JSCOV_DIR;
> +    var _CoverageCollector = Components.utils.import("resource://testing-common/CoverageUtils.jsm", {}).CoverageCollector;
> +    var coverageCollector = new _CoverageCollector(jscov_output_dir);
> +  }

I think we can use `let` instead of `var` in this file.

::: testing/xpcshell/head.js:522
(Diff revision 1)
> +  var coverageCollector = null;
> +  if (typeof _JSCOV_DIR === 'string') {
> +    var jscov_output_dir = _JSCOV_DIR;
> +    var _CoverageCollector = Components.utils.import("resource://testing-common/CoverageUtils.jsm", {}).CoverageCollector;
> +    var coverageCollector = new _CoverageCollector(jscov_output_dir);
> +  }

Would it be possible to move this above where we load the test files? It seems like it would be possible to have some interesting things happen at the top level of a test file when it's loaded.

::: testing/xpcshell/head.js:523
(Diff revision 1)
>        return ("_only" in props) && props._only;
>      });
>    }
>  
> +  var coverageCollector = null;
> +  if (typeof _JSCOV_DIR === 'string') {

This can just be `if (_JSCOV_DIR) {`

::: testing/xpcshell/head.js:524
(Diff revision 1)
>      });
>    }
>  
> +  var coverageCollector = null;
> +  if (typeof _JSCOV_DIR === 'string') {
> +    var jscov_output_dir = _JSCOV_DIR;

This variable doesn't seem necessary.

::: testing/xpcshell/head.js:1311
(Diff revision 1)
>      var testPath = do_get_file(testFile).path.replace(/\\/g, "/");
>      do_test_pending("run in child");
> +    if (typeof _JSCOV_DIR === 'string') {
> +      sendCommand("_testLogger.info('CHILD-TEST-STARTED'); "
> +                  + "const _TEST_FILE=['" + testPath + "']; "
> +                  + "const _JSCOV_DIR=" + uneval(_JSCOV_DIR) + ";"

Does this belong in the `do_load_child_test_harness` function?
https://reviewboard.mozilla.org/r/58506/#review56774

I'll add the mozharness bits in the next revision.

> Does this belong in the `do_load_child_test_harness` function?

You're right, I'll have this moved.
https://reviewboard.mozilla.org/r/58506/#review56774

> This can just be `if (_JSCOV_DIR) {`

I am getting an error when I use it this way if _JSCOV_DIR is undefined using | mach xpcshell-test --sequential |: https://pastebin.mozilla.org/8879236 . It does work though when I set _JSCOV_DIR to a directory.
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58506/diff/1-2/
Attachment #8761212 - Flags: review?(cmanchester)
Attachment #8761211 - Attachment is obsolete: true
Attachment #8761212 - Flags: review?(cmanchester)
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

https://reviewboard.mozilla.org/r/58506/#review57360

Looking good, just some small nits here. Before we go to land this, can you post a link in the bug to a try push with this running so we can see the results?

::: testing/mozharness/configs/unittests/linux_unittest.py:281
(Diff revision 2)
>                          "--manifest=tests/xpcshell/tests/xpcshell.ini"],
>              "tests": []
>          },
> +        "xpcshell-coverage": {
> +            "options": ["--xpcshell=%(abs_app_dir)s/" + XPCSHELL_NAME,
> +                        "--tag=addons",

We don't want "--tag=addons" for this suite.

::: testing/mozharness/scripts/desktop_unittest.py:389
(Diff revision 2)
>              str_format_values['test_plugin_path'] = abs_res_plugins_dir
>  
>              if suite_category not in c["suite_definitions"]:
>                  self.fatal("'%s' not defined in the config!")
>  
> -            if suite == 'browser-chrome-coverage':
> +            if suite == 'browser-chrome-coverage' or suite == 'xpcshell-coverage'::

This can be `if suite in ('browser-chrome-coverage', 'xpcshell-coverage')`.

::: testing/xpcshell/runxpcshelltests.py:1192
(Diff revision 2)
>          self.mozInfo = mozInfo
>          self.testingModulesDir = testingModulesDir
>          self.pluginsPath = pluginsPath
>          self.sequential = sequential
>          self.failure_manifest = failure_manifest
> +        self.jscovdir=jscovdir

Put spaces around `=`.
Here is a link to a try push which uses the changes and collects code coverage:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c9f5b8b43f4c00439558b961210699a460442ee
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58506/diff/2-3/
Attachment #8761212 - Flags: review?(cmanchester)
Attachment #8761212 - Flags: review?(cmanchester) → review+
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

https://reviewboard.mozilla.org/r/58506/#review57452

Looking good, thank you for the patch, Greg.
Greg, to get this landed, you can set the "checkin-needed" keyword on the bug. The commit message needs a little fixup before that though, it looks like you have both commit messages folded into one, and you want "r=chmanchester" at the end of the first line.
Comment on attachment 8761212 [details]
Bug 1278649 - Add code coverage to xpcshell tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58506/diff/3-4/
checkin-needed
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d5ef72784b
Add code coverage to xpcshell tests. r=chmanchester
Keywords: checkin-needed

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77d5ef72784b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.