811409 - Run desktop C++ unit tests from test package

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Currently we only run C++ unit tests during "make check". We should run them on the test machines from the test package instead.

Comment 1

[(not currently active) Ted Mielczarek]

If we fix bug 811411, we could also run the C++ unit tests on mobile, which would get us better test coverage.

Comment 2

[Dan Minor [:dminor]]

Attachment: Patch to stage cpptests as part of package-tests.

Work in progress to stage cpptests. Next step will be to move logic to run tests out of 'make check'.

Comment 3

[(not currently active) Ted Mielczarek]

I wouldn't worry about removing them from make check yet. Once we've got them split off into a separate job we can deal with that.

Comment 4

[(not currently active) Ted Mielczarek]

(Also, apparently this patch belongs on bug 811404, FWIW.)

Comment 5

[Dan Minor [:dminor]]

Attachment: Patch to run cppunittests from mozharness.

Work in progress to run desktop cppunittests.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 769149 [details] [diff] [review]
Patch to run cppunittests from mozharness.

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

::: scripts/desktop_unittest.py
@@ +316,5 @@
> +        abs_app_plugins_dir = os.path.join(abs_app_dir, 'plugins')
> +        if suites:  # there are cppunittest suites to run
> +            cppunittests = map(lambda f: os.path.join(dirs['abs_cppunittest_dir'], f), glob.glob(os.path.join(dirs['abs_cppunittest_dir'], '*')))
> +            for i in xrange(0, len(cppunittests)):
> +                suites['cppunittest%d' % i] = [cppunittests[i]] 

This is...a little weird. You're setting up sub-suites for each thing in the tests dir?

I guess this is because runcppunittests.py expects to get binary names on the commandline? We could pretty easily change it to accept either filenames or directory names, and just pass the directory here and force it to enumerate the contents. (Seems like that would simplify this.)

@@ +361,5 @@
>                      env['MINIDUMP_STACKWALK'] = c['minidump_stackwalk_path']
>                  if c.get('minidump_save_path'):
>                      env['MINIDUMP_SAVE_PATH'] = c['minidump_save_path']
> +                if c.get('ld_library_path'):
> +                    env['LD_LIBRARY_PATH'] = c['ld_library_path']

runcppunittests.py should already be taking care of this, no?
http://mxr.mozilla.org/mozilla-central/source/testing/runcppunittests.py#93

Comment 7

[Dan Minor [:dminor]]

I set up sub-suites so that runcppunittests.py is invoked for each executable file individually as it gave more stable test runs than attempting to run all of the tests from the same call to runcppunittests.py. I haven't (yet) spent the time to determine as to why this would be the case.

I wasn't aware that runcppunittests was attempting to set up LD_LIBRARY_PATH. It doesn't work properly (at least with mozharness), but I'll try to fix it there rather than through the mozharness script.

Comment 8

[(not currently active) Ted Mielczarek]

Let's scope this to only cover desktop tests for now, we can do Android tests as a followup. We know the desktop tests work since we already run them. We don't run the tests on Android, so bringing them up might involve disabling or fixing some of them.

Comment 9

[Dan Minor [:dminor]]

Attachment: Patch to add support for running c++ unittests to desktop_unittest.py.

I'll also attach a sample config file that I've been using for local testing.

Comment 10

[Dan Minor [:dminor]]

Attachment: Sample config file to run cpp unittests on linux.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 774606 [details] [diff] [review]
Patch to add support for running c++ unittests to desktop_unittest.py.

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

::: scripts/desktop_unittest.py
@@ +317,5 @@
> +            #copy shared libs from application to test bin dir
> +            libs = glob.glob(os.path.join(abs_app_dir, '*.so'))
> +            for lib in libs:
> +                shutil.copy2(os.path.join(abs_app_dir, lib),
> +                             os.path.join(dirs['abs_test_bin_dir']))

You shouldn't need to do this, you should be able to just call runcppunittests with --xre-path=abs_app_dir. This is exactly what we do for running them in-tree:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#179

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 774609 [details]
Sample config file to run cpp unittests on linux.

Ah, I see, you just have a wrong xre path here. This wants to be the app dir, I don't know how you'd represent that in a mozharness config. (xre stands for XUL Runtime Environment, but basically it's just wherever libxul is.)

Comment 13

[Dan Minor [:dminor]]

Ted: thanks! I was under the mistaken impression that --xre-path had to point to where xulrunner lives, not libxul. I'll update the patch.

Comment 14

[Dan Minor [:dminor]]

Attachment: Updated sample linux config to run cpp unittests.

Comment 15

[Dan Minor [:dminor]]

Attachment: Patch to add support for running c++ unittests to desktop_unittest.py.

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 774654 [details] [diff] [review]
Patch to add support for running c++ unittests to desktop_unittest.py.

mozcrash depends on mozfile+mozlog, so I think it needs to come after those two in the list.

Comment 17

[Dan Minor [:dminor]]

Attachment: Patch to add support for running c++ unittests to desktop_unittest.py.

Thanks! Revised version of the patch.

Comment 18

[Aki Sasaki (not active)]

Comment on attachment 775766 [details] [diff] [review]
Patch to add support for running c++ unittests to desktop_unittest.py.

https://hg.mozilla.org/build/mozharness/rev/9abede48fb56

Comment 19

[Aki Sasaki (not active)]

Backed out due to bustage, e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=25301015&tree=Mozilla-Aurora
:

15:53:16 ERROR - Return code: 1
15:53:17 ERROR - Return code: 1
16:04:52 FATAL - Uncaught exception: Traceback (most recent call last):
16:04:52 FATAL - File "C:\slave\test\scripts\mozharness\base\script.py", line 1005, in run
16:04:52 FATAL - self.run_action(action)
16:04:52 FATAL - File "C:\slave\test\scripts\mozharness\base\script.py", line 947, in run_action
16:04:52 FATAL - self._possibly_run_method(method_name, error_if_missing=True)
16:04:52 FATAL - File "C:\slave\test\scripts\mozharness\base\script.py", line 888, in _possibly_run_method
16:04:52 FATAL - return getattr(self, method_name)()
16:04:52 FATAL - File "scripts/scripts/desktop_unittest.py", line 286, in run_tests
16:04:52 FATAL - self._run_category_suites('cppunittest')
16:04:52 FATAL - File "scripts/scripts/desktop_unittest.py", line 312, in _run_category_suites
16:04:52 FATAL - abs_base_cmd = self._query_abs_base_cmd(suite_category)
16:04:52 FATAL - File "scripts/scripts/desktop_unittest.py", line 192, in _query_abs_base_cmd
16:04:52 FATAL - run_file = c['run_file_names'][suite_category]
16:04:52 FATAL - KeyError: 'cppunittest'
16:04:52 FATAL - Exiting -1

Comment 20

[Dan Minor [:dminor]]

Sorry I missed that. Looks like this can't be landed without updating the configuration files as wel (or adding code to handle a missing suite_category)

Comment 21

[Dan Minor [:dminor]]

Attachment: Patch to desktop_unittests.py and config files to run c++ unit tests.

Ash run for these changes is here: https://tbpl.mozilla.org/?tree=Ash&rev=87e0aaba801d. (The Talos failures are due to me forgetting to update ash-mozharness to the latest mozharness prior to applying my patch.)

Comment 22

[Aki Sasaki (not active)]

Comment on attachment 781686 [details] [diff] [review]
Patch to desktop_unittests.py and config files to run c++ unit tests.

Thanks!
It wouldn't hurt to watch Cedar after this lands on default, and inbound once it lands on production, since we had issues the first time around.  The Ash run helps a lot, though.

Comment 23

[Aki Sasaki (not active)]

Comment on attachment 781686 [details] [diff] [review]
Patch to desktop_unittests.py and config files to run c++ unit tests.

http://hg.mozilla.org/build/mozharness/rev/3605ab526175

Comment 24

[Justin Wood (:Callek)]

In production

Comment 25

[Dan Minor [:dminor]]

We're now running from the test package and not from make check.