1278649 - add support to the xpcshell harness for code coverage collection

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 1

[Greg Mierzwinski [:sparky]]

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

Comment 2

[Greg Mierzwinski [:sparky]]

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

[Chris Manchester (limited bugmail, email directly)]

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

Comment 4

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 5

[Greg Mierzwinski [:sparky]]

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.

Comment 6

[Greg Mierzwinski [:sparky]]

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 7

[Greg Mierzwinski [:sparky]]

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/

Comment 8

[Chris Manchester (limited bugmail, email directly)]

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 `=`.

Comment 9

[Greg Mierzwinski [:sparky]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b5acafcb3d0

Comment 10

[Greg Mierzwinski [:sparky]]

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 11

[Greg Mierzwinski [:sparky]]

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/

Comment 12

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

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 14

[Greg Mierzwinski [:sparky]]

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/

Comment 15

[Greg Mierzwinski [:sparky]]

checkin-needed

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77d5ef72784b
Add code coverage to xpcshell tests. r=chmanchester

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77d5ef72784b