Add mach target for cpp unittests

RESOLVED FIXED in mozilla29

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: dminor, Assigned: dminor)

Tracking

(Blocks 1 bug)

unspecified
mozilla29
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
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.
Blocks: machfeatures
Component: mach → Build Config
Assignee

Comment 1

6 years ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #8349640 - Flags: review?(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.
Attachment #8349640 - Flags: review?(gps) → feedback+
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'?
Assignee

Comment 4

6 years ago
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
Attachment #8349640 - Attachment is obsolete: true
Attachment #8355225 - Flags: review?(gps)
Comment on attachment 8355225 [details] [diff] [review]
Patch to add mach command for cppunittests

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

LGTM.
Attachment #8355225 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/8ffb10cf6138
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.