949538 - Add mach target for cpp unittests

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dan Minor [:dminor]]

If we are going to remove cpp unittests from 'make check' we should provide a mach target for people to easily run the tests locally.

Comment 1

[Dan Minor [:dminor]]

Attachment: Add mach command for cpp unittests

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8349640 [details] [diff] [review]
Add mach command for cpp unittests

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

Just nits. Next review should get a quick r+.

::: testing/mach_commands.py
@@ +165,5 @@
>          return 1
> +
> +@CommandProvider
> +class MachCommands(MachCommandBase):
> +    @Command('cppunittest', category='testing',

Grumbles about the mixed convention between "FOOtest" and "FOO-test". This is fine, I suppose.

@@ +168,5 @@
> +class MachCommands(MachCommandBase):
> +    @Command('cppunittest', category='testing',
> +        description='Run cpp unit tests.')
> +    @CommandArgument('--symbols-path', nargs='?', metavar='TEST',
> +        help = 'Absolute path to directory containing breakpad symbols, or the URL of a zip file containing symbols')

It would be cool if we could grab the locally-generated symbols automagically if they are available. Followup fodder.

@@ +172,5 @@
> +        help = 'Absolute path to directory containing breakpad symbols, or the URL of a zip file containing symbols')
> +    @CommandArgument('test_files', nargs='?', metavar='N',
> +        help='Test to run. Can be specified as one or more files or ' \
> +            'directories, or omitted. If omitted, the entire test suite is ' \
> +            'executed.')

nargs='?' will only allow 0 or 1 arguments. I think you want '*'

@@ +178,5 @@
> +    def run_cppunit_test(self, **params):
> +        import runcppunittests as cppunittests
> +        from mozbuild.controller.building import BuildDriver
> +
> +        driver = self._spawn(BuildDriver)

I don't think you need to import the driver like this. You should be able to access self.distdir and self.bindir.

@@ +187,5 @@
> +        else:
> +            progs = cppunittests.extract_unittests_from_args(params['test_files'], None)
> +
> +        tester = cppunittests.CPPUnitTests()
> +        result = tester.run_tests(progs, driver.bindir, params.get('symbols_path', None))

You should surround this with a try..except Exception, otherwise mach displays an unhelpful message about it likely being a bug in the command method. There probably needs to be a mach command option to have uncaught exceptions caught intelligently. Patches welcome.

::: testing/runcppunittests.py
@@ +163,5 @@
>              progs.extend([os.path.abspath(os.path.join(p, x)) for x in os.listdir(p) if not x in skipped_progs])
>          elif p not in skipped_progs:
>              progs.append(os.path.abspath(p))
>  
> +    return progs 

Nit: trailing whitespace.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8349640 [details] [diff] [review]
Add mach command for cpp unittests

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

::: testing/mach_commands.py
@@ +43,5 @@
>  
>  TEST_SUITES = {
> +    'cppunittest': {
> +        'mach_command': 'cppunittest',
> +        'kwargs': {'test_file': None},

Can you add an alias to 'Cpp'?

Comment 4

[Dan Minor [:dminor]]

Attachment: Patch to add mach command for cppunittests

Thanks! Revised to address the comments.

A try run to make sure I didn't break running things from the test package:
https://tbpl.mozilla.org/?tree=Try&rev=1db221b17fd8&showall=1

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8355225 [details] [diff] [review]
Patch to add mach command for cppunittests

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

LGTM.

Comment 6

[Dan Minor [:dminor]]

Thanks! Pushed to https://hg.mozilla.org/integration/mozilla-inbound/rev/8ffb10cf6138

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ffb10cf6138