1429463 - Prototype ./mach try coverage tool

Status: RESOLVED FIXED

(Testing :: Code Coverage, enhancement)

Description

[Kyle Lahnakoski [:ekyle]]

We would like to run tests based on the files modified by a patch. It would seem code coverage can provide us with such a mapping.  Right now coverage is at the chunk level, so we can choose chunks hat would cover a given change.  We can improve resolution by running overage by directory, or coverage by test.

Comment 1

[Marco Castelluccio [:marco]]

For reference:
(In reply to Joel Maher ( :jmaher) (UTC-5) from bug 1429455 comment #1)
> for work outside the scope here, I wanted to reference my try push that
> shows collecting coverage for a single test:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=182b519971d39b8c66c6faf61f62b87973d81608
> 
> and the code:
> https://hg.mozilla.org/try/rev/683e50ab5eaab7945d8cd6573d5ecd37634f3141

Comment 2

[Marco Castelluccio [:marco]]

I've written a PoC script to generate the mapping in a sqlite3 database: https://github.com/marco-c/code-coverage-reports/blob/master/chunk_mapping.tar.xz.

The file is small, so it can easily be downloaded locally when running mach try.

You can test it by copying and pasting a patch at this URL: https://marco-c.github.io/code-coverage-reports/chunks.html.

Comment 3

[Marco Castelluccio [:marco]]

The next step is generating a mapping between chunks and directories/manifests, since the chunks are not stable (they change multiple times per day) and they are always going to be different anyway between different kinds of build (e.g. there are more chunks on ccov than opt).

Comment 4

[Marco Castelluccio [:marco]]

The file at https://github.com/marco-c/code-coverage-reports/blob/master/chunk_mapping.tar.xz now contains both a mapping between source file and chunk and a mapping between chunk and tests run.

Comment 5

[Kyle Lahnakoski [:ekyle]]

This makes me very happy! A database! Excellent work! 

Can the file extension be ".sqlite" instead of ".db"?

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Add a 'coverage' try selector

Here's a basic implementation (with several things still left to be figured out) of a 'coverage' try selector.

TODO:
1) Figure out what to do when a file with no coverage information is modified;
2) Host the mapping file as an artifact of the taskcluster task that generated it, instead of on GitHub;
3) Figure out chunking (bug 1434914).

For 1) I would implement some simple heuristics:
- Are you modifying a test file? Only run the modified test.
- Are you modifying a test support file in a test directory? Only run the tests in that directory.
- Are you modifying a file with no coverage information that doesn't match the earlier two? Run everything.

There might be failures that we ignore with these heuristics (e.g. a change in a test causes another test to fail because they are somehow connected), but this is always going to be true when using coverage information to select tests.

For 3), James suggested:
> <jgraham> So one way to get the chunking to work would be to change the algorithm to apply test selection as a post-chunking filter
> <jgraham> But that would certainly require changes to all the harnesses
> <jgraham> And some magic to eliminate empty chunks
> <jgraham> But it does have the advantage that the chunks you see on try are the same as the ones on central

Comment 7

[Marco Castelluccio [:marco]]

Another TODO:
4) On which platforms should we run the tests? I think just on the fastest possible linux64 and windows64 (opt?).

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956118 [details] [diff] [review]
Add a 'coverage' try selector

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

Thanks, this looks good to me. Exciting to see this getting close!

::: tools/tryselect/selectors/coverage.py
@@ +27,5 @@
> +    common_groups = ['push']
> +    templates = ['artifact', 'env', 'rebuild']
> +
> +
> +def filter_by_paths(tasks, paths):

We should refactor this out of here and fuzzy.py into a common area.

@@ +55,5 @@
> +            etag = f.read()
> +    except IOError:
> +        etag = ''
> +
> +    CHUNK_MAPPING_URL = 'https://raw.githubusercontent.com/marco-c/code-coverage-reports/master/chunk_mapping.tar.xz'  # noqa

I guess this is just the chunk mapping for mozilla-central tip? Maybe we can upload these files as taskcluster artifacts and we can find the appropriate one based on the user's local revision.

@@ +74,5 @@
> +
> +def run_coverage_try(templates=None, full=False, parameters=None,
> +                     push=True, message='{msg}',
> +                     paths=None, **kwargs):
> +    download_coverage_mapping()

Probably a bit cleaner if this returns the path to the DB

@@ +102,5 @@
> +    path_env = {'MOZHARNESS_TEST_PATHS': paths}
> +    if templates is None:
> +        templates = {}
> +    if 'env' not in templates:
> +        templates['env'] = {}

You can use `templates.setdefault('env', {})` here. Could even chain the `path_env` update to the same line if you wanted.

Comment 9

[Marco Castelluccio [:marco]]

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Comment on attachment 8956118 [details] [diff] [review]
> Add a 'coverage' try selector
> 
> Review of attachment 8956118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks good to me. Exciting to see this getting close!
> 
> ::: tools/tryselect/selectors/coverage.py
> @@ +27,5 @@
> > +    common_groups = ['push']
> > +    templates = ['artifact', 'env', 'rebuild']
> > +
> > +
> > +def filter_by_paths(tasks, paths):
> 
> We should refactor this out of here and fuzzy.py into a common area.
> 
> @@ +55,5 @@
> > +            etag = f.read()
> > +    except IOError:
> > +        etag = ''
> > +
> > +    CHUNK_MAPPING_URL = 'https://raw.githubusercontent.com/marco-c/code-coverage-reports/master/chunk_mapping.tar.xz'  # noqa
> 
> I guess this is just the chunk mapping for mozilla-central tip? Maybe we can
> upload these files as taskcluster artifacts and we can find the appropriate
> one based on the user's local revision.

I've filed https://github.com/mozilla-releng/services/issues/769 a while ago about this, but haven't worked on it yet.

Comment 10

[Gabriel Vîjială [:gabriel-v]]

I'll pick this up at Marco's request.

Marco: the github '*/chunk_mapping.tar.xz' links are dead. Since https://github.com/mozilla-releng/services/issues/769 is fixed, I guess there will be an endpoint working on the backend. Where can I find what that endpoint is?

Also, I will have to find the user's local parent revision (the first one that is on mozilla-central). Do I use "subprocess.run(['hg', ...])" to get it, or do we have some python package that already implements that logic?

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #10)
> I'll pick this up at Marco's request.
> 
> Marco: the github '*/chunk_mapping.tar.xz' links are dead. Since
> https://github.com/mozilla-releng/services/issues/769 is fixed, I guess
> there will be an endpoint working on the backend. Where can I find what that
> endpoint is?

They are now generated as Taskcluster artifacts of the code coverage tasks. You can find the tasks via the Tasckluster index, e.g. https://tools.taskcluster.net/index/project.releng.services.project.production.shipit_code_coverage/04dd259d71db60341016eccf53ced43742319631.
The revision (04dd259d71db60341016eccf53ced43742319631) is the Mercurial revision of the mozilla-central code coverage build.

> Also, I will have to find the user's local parent revision (the first one
> that is on mozilla-central). Do I use "subprocess.run(['hg', ...])" to get
> it, or do we have some python package that already implements that logic?

We have a VCSHelper at tools/tryselect/vcs.py, but it doesn't have the logic you need (though it does have something similar in `files_changed`). You can add the new function to the helper.

A note to keep in mind while working on this: not all mozilla-central revisions have a chunk mapping file, so we will have to try with older revisions when the latest doesn't have it.

Comment 12

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #11)
> We have a VCSHelper at tools/tryselect/vcs.py, but it doesn't have the logic
> you need (though it does have something similar in `files_changed`). You can
> add the new function to the helper.

Looks like this module was removed and we're now using mozversioncontrol in try selectors.

Comment 13

[Marco Castelluccio [:marco]]

An additional note:
The chunk mapping file now contains three tables instead of just two, as it contains per-test data too now.
The per-test data is in the file_to_test table.

The patch should be modified a little to merge the list of tests coming from per-test data (limited to suites that support running in per-test mode) and the list of tests coming from generic coverage data. The retrieval of the second list from the database is already part of the patch. What needs to be added is the retrieval of the first list from the file_to_test table.

Comment 14

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1429463 - Prototype ./mach try coverage tool.

Prototype ./mach try coverage tool.

Comment 15

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8999216 [details]
Bug 1429463 - Prototype ./mach try coverage tool.

I updated Marco's patch. There are still two issues I to figure out:

- (first TODO) I need to find if a given file is a support file, and if so, what tests it supports. Could I do this by calling TestResolver.resolve_tests with an argument of `cwd` being the parent directory of the possible support file? 

- For some test suites, we have per-test coverage, so we know exactly what tests need to be run, and we add those to MOZHARNESS_TEST_PATHS. For other test suites, we only get the platform and chunk name. These are in non-canonical format, as the test suite names come from ActiveData.

For this second point, the platform names are:
- windows
- linux

And the suite names are:
- cppunit (should be cppunittest)
- firefox-ui-functional-local (should be firefox-ui-functional)
- firefox-ui-functional-remote (should be firefox-ui-functional)
- gtest (not known by the TestResolver)
- jittest-1 (not known by the TestResolver)
- jittest-2
- jittest-3
- jittest-4
- jittest-5
- jittest-6
- marionette
- marionette-headless

The first problem would be identifying the tasks from these platform/suite combinations. I would probably need to set up some translations (for the 'should be' entries).

The second problem is this (second TODO): if a task listed in try_task_config.json does not have any respective paths in the MOZHARNESS_TEST_PATHS, will it run no tests, or all tests? If it doesn't run all tests, could we make it do that?

Comment 16

[Gabriel Vîjială [:gabriel-v]]

This depends on https://github.com/mozilla/release-services/pull/1401, because our chunk mapping artifacts are truncated.

Here's a try push with one support file changed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cff4cdd2fae5e2dda108e1defa5d3a5afaee29c

I'll submit more try pushes with different kinds of tests being modified.

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8999216 [details]
Bug 1429463 - Prototype ./mach try coverage tool.

Andrew Halberstadt [:ahal] has approved the revision.

Comment 18

[Marco Castelluccio [:marco]]

Comment on attachment 8999216 [details]
Bug 1429463 - Prototype ./mach try coverage tool.

Marco Castelluccio [:marco] has approved the revision.

Comment 19

[Gabriel Vîjială [:gabriel-v]]

I've made a list of issues regarding this: https://gist.github.com/gabriel-v/66e23d30f6a470a1b714f3e77dfa3bfe

I will transfer these to bugzilla and update the document with bug numbers.

Comment 20

[Gabriel Vîjială [:gabriel-v]]

So the situation with this patch is:

1. Compiled test suites (gtest, cppunit, jittest) don't work with other test paths on their MOZHARNESS_TEST_PATH
2. Some test suites (marionette, crashtests) also don't work with MOZHARNESS_TEST_PATHS
3. Test verification tasks fail when they see an unrecognized file on MOZHARNESS_TEST_PATHS
4. Most tasks, when scheduled to run 0 tests (because they're all skipped on the given platform), will fail


gbrown, ahal: what behaviour do we want from all of these test suites when pushing to try with MOZHARNESS_TEST_PATHS set? In order to make 'mach try coverage' work with one push for all tests, we need all of these suites to accept the same MOZHARNESS_TEST_PATHS from the same try_task_config.json.


Could we use multiple lists with paths, say one for each suite? That way, there won't be any confusion between suites, like there is in https://bugzilla.mozilla.org/show_bug.cgi?id=1488850#c1. This could be a set of MOZHARNESS_<suitename>_TEST_PATHS environment variables to replace the single MOZHARNESS_TEST_PATHS. Or even better, another dict in try_task_config.json: {'test_paths': {'<suitename>': ['path1', 'path2']}, 'tasks': ['task1', 'task2']}.


If we don't want to change how try_task_config.json is structured, then for 1, could we make the compiled suites to completely ignore the MOZHARNESS_TEST_PATH? It seems to be badly interpreted in all examples I tried. I also had no success in running any of the compiled tests with a path prefix, with 'mach try fuzzy PATH'.


3 should be fixed by using separate paths lists for each suite, proposed above.
4 should be fixed by filtering out those tasks before pushing them to try, in bug 1488849

Comment 21

[Andrew Halberstadt [:ahal]]

Thanks, nice write-up!

(In reply to Tudor-Gabriel Vijiala [:tvijiala] from comment #20)
> So the situation with this patch is:
> 
> 1. Compiled test suites (gtest, cppunit, jittest) don't work with other test
> paths on their MOZHARNESS_TEST_PATH

I already commented in bug 1488842, but I see your try push uses artifact builds. Compiled tests are expected to fail in that case, so I'd like to see a try run with normal builds (pass --no-artifact) to make sure this is actually a problem.

> 2. Some test suites (marionette, crashtests) also don't work with
> MOZHARNESS_TEST_PATHS

I thought these two should work, but if not we should fix this in the mozharness scripts.


> 4. Most tasks, when scheduled to run 0 tests (because they're all skipped on
> the given platform), will fail

Commented in bug 1488849


> Could we use multiple lists with paths, say one for each suite? That way,
> there won't be any confusion between suites, like there is in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1488850#c1. This could be a set
> of MOZHARNESS_<suitename>_TEST_PATHS environment variables to replace the
> single MOZHARNESS_TEST_PATHS. Or even better, another dict in
> try_task_config.json: {'test_paths': {'<suitename>': ['path1', 'path2']},
> 'tasks': ['task1', 'task2']}.

I like the idea of modifying try_task_config.json (instead of separate environment variables). We'll have to double check with jgraham to make sure the external wpt sync bot isn't depending on the old format.

Comment 22

[Gabriel Vîjială [:gabriel-v]]

(In reply to Andrew Halberstadt [:ahal] from comment #21)
> I like the idea of modifying try_task_config.json (instead of separate
> environment variables). We'll have to double check with jgraham to make sure
> the external wpt sync bot isn't depending on the old format.

I just asked him about this. He said wpt sync doesn't depend on the try_task_config.json format. He also said he supports changing the format to separate the test lists for each suite, since they have had issues with that in the past.

I'm opening a new bug to change the try_task_config format.

Comment 23

[Geoff Brown [:gbrown]]

Good discussion so far. The only thing I would add is that it might be advantageous to include the origin of the request along with the paths. Right now there's a problem when TV gets a MOZHARNESS_TEST_PATH that specifies a non-test (or a test that TV doesn't recognize): If the path was requested from the test-verify-backfill UI, we want TV to fail loudly; if it came from 'mach try', we probably want different behavior.

Comment 24

[Gabriel Vîjială [:gabriel-v]]

(In reply to Geoff Brown [:gbrown] from comment #23)
> Good discussion so far. The only thing I would add is that it might be
> advantageous to include the origin of the request along with the paths.
> Right now there's a problem when TV gets a MOZHARNESS_TEST_PATH that
> specifies a non-test (or a test that TV doesn't recognize): If the path was
> requested from the test-verify-backfill UI, we want TV to fail loudly; if it
> came from 'mach try', we probably want different behavior.

If 'mach try' is made to specify test paths separately for each test suite as per bug 1489100, TV will only look at the paths it should recognize.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab76c815c205
Prototype ./mach try coverage tool. r=ahal,marco
https://hg.mozilla.org/integration/mozilla-inbound/rev/9add1776dcaa
Use new Taskcluster index URL for the code coverage bot. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/1368e9cf9344
Use 'chunk' variable name instead of 'suite'. r=me

Comment 26

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ab76c815c205
https://hg.mozilla.org/mozilla-central/rev/9add1776dcaa
https://hg.mozilla.org/mozilla-central/rev/1368e9cf9344