1047575 - Mach command to parse manifests and list skipped tests

Status: RESOLVED INACTIVE

(Firefox Build System :: Mach Core, enhancement)

Description

[Ahmed Kachkach [:akachkach]]

We should have a mach command that shows the state of test manifests (skipped tests/test coverage on the current platform). Will probably use some of the code from : https://github.com/halflings/manifestmonitor

Hopefully we'll also make a small web app to track the evolution of test coverage on all platforms.

Comment 1

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

A WIP patch. Working, but I have a couple manifests (reftest and crashtest) that can't be parsed (because they should use as their relative root the srcdir and not the manifest's dir, and I can't find an option in manifestparser.TestManifest to do that.

Comment 2

[Ahmed Kachkach [:akachkach]]

These are the errors I get when parsing some manifests:

! Couldn't parse manifest 'reftest/reftest.ini':
Included file '/Users/akachkach/workspace/gecko-dev/obj-x86_64-apple-darwin14.0.0/_tests/reftest/layout/reftests/reftest.list' does not exist
--------------------------------------------------------------------------------
! Couldn't parse manifest 'crashtest/crashtest.ini':
Included file '/Users/akachkach/workspace/gecko-dev/obj-x86_64-apple-darwin14.0.0/_tests/crashtest/testing/crashtest/crashtests.list' does not exist

Maybe the issue here isn't so much the fact the the path is relative to the srcdir, but the fact that those included manifests should also be in the objdir/_tests/ directory?

Comment 3

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

Comment on attachment 8466705 [details] [diff] [review]
0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

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

this seems to be a lot of pretty human readable formatting, how can we make this easy to get into a database?

::: testing/mach_commands.py
@@ +319,5 @@
>  
>          return all_passed
> +
> +
> +MANIFESTS_REL_PATH = ['testing/mochitest/tests/mochitest.ini', 'reftest/reftest.ini', 'crashtest/crashtest.ini',

so reftest and crashtest use a different format, it is reftest.list and crashtest.list.  The parser is custom to the reftest harness.

Comment 4

[(not currently active) Ted Mielczarek]

reftests and crashtests don't use manifestparser-style manifests, they use reftest-style manifests:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/reftest.list
http://mxr.mozilla.org/mozilla-central/source/testing/crashtest/crashtests.list?force=1

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/ffaf206fd018 added a Python reftest parser to the tree. Note that conditions for reftests are evaluated in JavaScript, so calculating the set of skipped tests from pure Python is not realistic.

Comment 6

[Andrew Halberstadt [:ahal]]

I think it's ok to focus on manifestparser style manifests for now. Just make sure that it's possible to easily swap out parsers at run time depending on what suite is getting checked.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8466705 [details] [diff] [review]
0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

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

Thanks, this looks good so far! A few comments inline.

::: testing/mach_commands.py
@@ +325,5 @@
> +@CommandProvider
> +class TestsState(MachCommandBase):
> +    @Command('tests-state', category='testing', description='Stats about skipped tests and test coverage in the current platform.')
> +    @CommandArgument('-os', choices=mozinfo.choices['os'],
> +        help='Operating system.')

I'm not really sure if these overrides are a good idea or not. I can see why they might be appealing, but I think they might lead to confusion. I can imagine a dev saying "But I specified 'os=win' and 'bits=32'.. how come I'm not getting the same list of tests that are running in tbpl?"

I think instead of only overriding select options, we should just have a single argument that let's us override the entire build config by specifying a path to a different mozinfo.json (or something that looks like a mozinfo.json file). It's less convenient, but it forces the user to know what they are doing and understand how the manifests are filtered.

@@ +329,5 @@
> +        help='Operating system.')
> +    @CommandArgument('-bits', choices=map(str, mozinfo.choices['bits']),
> +        help='Bits.')
> +    @CommandArgument('-processor', choices=mozinfo.choices['processor'],
> +        help='Processor.')

It would be nice if we could specify a test suite (or list of test suites) to print.

@@ +336,5 @@
> +        from manifestparser import TestManifest
> +        try:
> +            build = MozbuildObject.from_environment()
> +            srcdir = build.topsrcdir
> +            objdir = build.topobjdir

Instead of this I would add a condition to the @Command decorator. E.g 'conditions=[conditions.has_build]'. See [1] for an example of this. Note the 'has_build' condition doesn't currently exist, but you can add it to [2]. You can use "hasattr(cls, 'substs')" as the conditional.

The benefit of this is it will move the command to the 'disabled' group if the user doesn't have a build. If they try to run it, a message (coming from the condition docstring) explaining what they need to do will get printed.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#744
[2] http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#604

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8466705 [details] [diff] [review]
0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

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

IMO there is value in having this information printed during config.status (mach build-backend). If we do that, these stats will be printed in automation. That could help spot-check build system changes to quickly identify whether test counts changed during "innocent" changes. (We've had build system refactorings accidentally remove tests before.)

If you don't mind the scope bloat, you could stick the "get test stats" API into python/mozbuild/mozbuild/testing.py and we could get the config.status integration for nearly free, with very little code reuse. FWIW, I always hoped that mach_commands.py would be very thin - actual "business logic" code would live in importable modules, not mach_commands.py files. This would encourage reuse and testing.

Comment 9

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

As requested, I moved the common code to mozbuild.testing; After discussing this with ahal, I also removed the config parameters (os, bits and processor) and added a mozconfig parameter instead (path to a mozinfo.json file).

I integrated this with build-backend; should I put it in a separate bug instead?

Comment 10

[Gregory Szorc [:gps]]

If you wrote the code, by all means include it here. It will need build peer review though. Might be best to be in separate patch.

Comment 11

[Ahmed Kachkach [:akachkach]]

@gps: The code is included in the patch I included. I'm just going to try to add a "has_build" condition before I submit this for review.

Comment 12

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

Proposed patch. (also integrates the stats summary to the build-backend command)

@gps: Who should I ask for review from the build team?

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8468225 [details] [diff] [review]
0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

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

I'll review this since I'm a build peer.

::: python/mozbuild/mozbuild/base.py
@@ +607,5 @@
>      """
>      @staticmethod
> +    def has_build(cls):
> +        """Must have a build."""
> +        return hasattr(cls, 'substs')

I know you are copying existing code, but I'm not sure how hasattr(cls, 'substs') is an effective test, as MachCommandBase inherits from MozbuildObject and MozbuildObject has substs defined as a @property.

I would implement this as:

@staticmethod
def is_build_configured(cls):
    """"Whether the build system has been configured and is ready to run."""
    return self.topobjdir and os.path.exists(os.path.join(self.topobjdir, 'config.status'))

::: python/mozbuild/mozbuild/mach_commands.py
@@ +27,2 @@
>  
> +import mozinfo

mach_commands.py files are imported when mach runs. So, mozinfo will get imported when mach runs. mozinfo does stuff at module import time. This makes mach sad. Please delay import until inside the method that uses it.

@@ +548,5 @@
> +        if status == 0:
> +            objdir = self.topobjdir
> +            options = mozinfo.info
> +            stats = get_tests_stats(objdir, options)
> +            summarize_tests_stats(stats)

I should have been more clear in the requirements: `mach build-backend` is a wrapper around `config.status`. We'd want this stat printing inside config.status itself.

To do that, add some test attributes to the BackendConsumeSummary class at https://hg.mozilla.org/mozilla-central/file/bdf301b20cab/python/mozbuild/mozbuild/backend/base.py#l32.

Then, populate those attributes during test manifest processing. See https://hg.mozilla.org/mozilla-central/file/bdf301b20cab/python/mozbuild/mozbuild/backend/recursivemake.py#l1076.

Finally, add the new summary lines to BackendConsumeSummary.summaries() https://hg.mozilla.org/mozilla-central/file/bdf301b20cab/python/mozbuild/mozbuild/backend/base.py#l111 and they will magically be printed during config.status!

::: python/mozbuild/mozbuild/testing.py
@@ +35,5 @@
>          test['path'] = mozpath.join(new_base, test['file_relpath'])
>  
>      return test
>  
> +MANIFESTS_REL_PATH = ['testing/mochitest/tests/mochitest.ini', 'steeplechase/steeplechase.ini', 'xpcshell/xpcshell.ini']

Nit: Add newlines for long lists like this.

I'm not a huge fan of hard-coding these manifests here. The build config is the source of truth. We actually generate these unified manifests from the build config now!

You should be able to use the metadata collected during the build config evaluation instead of re-parsing test manifests. Unfortunately, that will require capturing some new data, as we e.g. currently throw away skipped tests.

@@ +37,5 @@
>      return test
>  
> +MANIFESTS_REL_PATH = ['testing/mochitest/tests/mochitest.ini', 'steeplechase/steeplechase.ini', 'xpcshell/xpcshell.ini']
> +def get_tests_stats(objdir, options):
> +    from manifestparser import TestManifest

This is a core mozbuild module, you don't need to delay import modules here.

@@ +38,5 @@
>  
> +MANIFESTS_REL_PATH = ['testing/mochitest/tests/mochitest.ini', 'steeplechase/steeplechase.ini', 'xpcshell/xpcshell.ini']
> +def get_tests_stats(objdir, options):
> +    from manifestparser import TestManifest
> +    result = dict()

result = {}

(the explicit dict() constructor is typically only used if you are passing arguments)

@@ +41,5 @@
> +    from manifestparser import TestManifest
> +    result = dict()
> +    result['options'] = options
> +    result['objdir'] = objdir
> +    stats = dict()

stats = {}

@@ +51,5 @@
> +            active_tests = {t['name'] for t in tests_manifest.active_tests(disabled=False, exists=False, **options)}
> +            skipped_tests = {t['name'] for t in all_tests if t['name'] not in active_tests}
> +            stats[manifest] = dict(active_tests=active_tests, skipped_tests=skipped_tests)
> +        except Exception as e:
> +            print("! Couldn't parse manifest '{}':\n{}".format(manifest, e))

Library code shouldn't print(): it should raise exceptions instead.

@@ +74,5 @@
> +        tests_n = len(active_tests) + len(skipped_tests)
> +        print("# Manifest     : {}".format(manifest))
> +        print("Active tests   : {}/{}".format(len(active_tests), tests_n))
> +        print("Coverage       : {:.2f}%".format(100.0*len(active_tests)/tests_n))
> +        print("-" * 80)

You'll need to refactor this for config.status to return strings instead of printing. Also, I generally shy away from straight printing from library code. Printing to a passed in file object, however, is OK.

::: testing/mach_commands.py
@@ +14,5 @@
>  )
>  
>  from mozbuild.base import MachCommandBase
> +from mozbuild.base import MachCommandConditions as conditions
> +import mozinfo

Another global mozinfo import that should be delayed.

Comment 14

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

WIP patch.

Comment 15

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

Working patch; I added the stats to BackendConsumeSummary, but aggregated by dir instead of by manifest (otherwise it's hard to see anything, as we process each individual manifest).

Comment 16

[Ahmed Kachkach [:akachkach]]

Is this the type of command that you and jgriffin wanted to track changing manifests?

Comment 17

[Andrew Halberstadt [:ahal]]

No, I think that would be hard to do in a mach command. We need to save snapshots over time, so that's what the web service would be for.

Comment 18

[Ahmed Kachkach [:akachkach]]

Sure, I discussed this with jgriffin yesterday.

In this case is it even useful to have this feature in mach? I'm not familiar with the work that sheriffs and devs can do on the tree, but I thought we also needed a command to see the state of the tree after applying a patch or changing some manifests.

Comment 19

[Andrew Halberstadt [:ahal]]

Hm, maybe I don't understand what jgriffin wants then.. Presumably if you apply a patch locally, you know (or can easily look up) whether it enables/disables any tests, so I'm not sure how useful that would be.

I think the mach command is still useful for developers curious about the state of tests. It wouldn't give them a complete picture or anything, but it would give them an idea. The mach command could also include a link to the webservice (or eventually invoke the webservice's theoretical API).

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8480884 [details] [diff] [review]
0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

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

Lots of nits. The mozinfo handling is my biggest complaint about this patch. Don't hesitate to ask a build peer for help if you need it.

::: python/mozbuild/mozbuild/backend/base.py
@@ +82,5 @@
>          # Mapping of changed file paths to diffs of the changes.
>          self.file_diffs = {}
>  
> +        # Statistics about test manifests (active/skipped tests in the current platform)
> +        self.manifest_stats = {"options": None, "stats": {}}

Let's make this "test_stats". "manifest" is overloaded in the build system.

@@ +113,5 @@
>                  self.other_time)
>  
> +    @property
> +    def manifests_summary(self):
> +        return summarize_tests_stats(self.manifest_stats)

tests_summary

@@ +139,5 @@
>      __metaclass__ = ABCMeta
>  
>      def __init__(self, environment):
>          assert isinstance(environment, ConfigEnvironment)
> +        import mozinfo

The magic in mozinfo that does foo on import needs to DIAF.

Can we hang the parsed mozinfo dict off ConfigEnvironment?

::: python/mozbuild/mozbuild/mach_commands.py
@@ +25,2 @@
>  )
> +from mozbuild.testing import get_tests_stats, summarize_tests_stats

It seems every change in this file can be discarded?

::: python/mozbuild/mozbuild/testing.py
@@ +56,5 @@
> +        try:
> +            tests_manifest = TestManifest([path])
> +            if not tests_manifest.tests:
> +                continue
> +            stats[manifest] = get_manifest_stats(tests_manifest, options)

I'm not a huge fan of having to parse the test manifests *again* in order to display stats.

We try to keep the run time of config.status down. Did you measure the impact of this patch?

@@ +58,5 @@
> +            if not tests_manifest.tests:
> +                continue
> +            stats[manifest] = get_manifest_stats(tests_manifest, options)
> +        except Exception as e:
> +            raise Exception("Couldn't parse manifest '{}':\n{}".format(manifest, e))

Does this ever happen? If we are producing a bad test manifest from the build backend, we're going to have a bad time. I'm not convinced this exception adds any value.

::: testing/mach_commands.py
@@ +324,5 @@
> +@CommandProvider
> +class TestStats(MachCommandBase):
> +    @Command('test-stats', category='testing', description='Stats about skipped tests and test coverage in the current platform.',
> +             conditions=[conditions.is_build_configured])
> +    @CommandArgument('-mozinfo', help='Path to a mozinfo.json file.')

I'm surprised '-mozinfo' works. I thought argparse enforced that '-' be used with single character arguments and '--' with multi-character arguments.

Please change to --mozinfo. If nothing else it is consistent with everything else.

@@ +325,5 @@
> +class TestStats(MachCommandBase):
> +    @Command('test-stats', category='testing', description='Stats about skipped tests and test coverage in the current platform.',
> +             conditions=[conditions.is_build_configured])
> +    @CommandArgument('-mozinfo', help='Path to a mozinfo.json file.')
> +    def tests_state(self, **params):

Please use mozinfo=None instead of **params.

@@ +328,5 @@
> +    @CommandArgument('-mozinfo', help='Path to a mozinfo.json file.')
> +    def tests_state(self, **params):
> +        import json
> +
> +        from mozbuild.base import MozbuildObject

This import can be at the file level.

@@ +332,5 @@
> +        from mozbuild.base import MozbuildObject
> +        from mozbuild.testing import get_tests_stats, summarize_tests_stats
> +        import mozinfo
> +
> +        objdir = MozbuildObject.from_environment().topobjdir

self.objdir should already be available.

Comment 21

[Jonathan Griffin (:jgriffin)]

IMO, the biggest need is to be able to see track differences in test (enabled/disabled) status over time.  Like ahal said, we need snapshots for that.

I think this mach command is useful for generating a high-level overview of test status, but I'm not sure how often it would get used.  The use case for testing the side effects of a patch is minimal, since the problem we'd be addressing is accidental disabling of large swaths of tests, which would depend on the patch author running this command with and without the patch and noting the differences.  In practice, I suspect it won't be used too often.

Comment 22

[Gregory Szorc [:gps]]

I *really* want build and mach-level visibility into test counts, especially skipped tests. Use cases:

1) Build system developer changes things and doesn't realize tests were accidentally disabled (this has happend before)
2) Developer runs tests in a directory and recognizes a surprisingly high amount of skipped tests.
3) Observers notice the skipped test percentage increase over time and start to get alarmed.

With skipped tests, I've had tests I care about disabled without me knowing. Maybe I wasn't following the right bug component or something. Printing info on skipped tests would increase visibility into skipped tests.

Along that same vein, we may also want to print the list of skipped tests when running tests locally through mach. We can do that with xpcshell tests today. mochitests is harder until we stop filtering skipped tests from the build system.

Comment 23

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1047575-mach-command-to-give-stats-about-test-co.patch

Sorry about the delay!

This solves most of the nits raised in the previous review, but still uses mozinfo.info to get the platform's config (couldn't find anything relevant in the ConfigEnvironment).

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Probably inactive!