Closed Bug 811409 Opened 8 years ago Closed 7 years ago

Run desktop C++ unit tests from test package

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: dminor)

References

Details

Attachments

(1 file, 7 obsolete files)

Currently we only run C++ unit tests during "make check". We should run them on the test machines from the test package instead.
If we fix bug 811411, we could also run the C++ unit tests on mobile, which would get us better test coverage.
See Also: → 811411
Work in progress to stage cpptests. Next step will be to move logic to run tests out of 'make check'.
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.
(Also, apparently this patch belongs on bug 811404, FWIW.)
Attachment #761409 - Attachment is obsolete: true
Work in progress to run desktop cppunittests.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
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
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.
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.
Summary: Run C++ unit tests from test package → Run desktop C++ unit tests from test package
Depends on: 892439
I'll also attach a sample config file that I've been using for local testing.
Attachment #769149 - Attachment is obsolete: true
Attachment #774606 - Flags: review?(aki)
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 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.)
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.
Attachment #774606 - Flags: review?(aki)
Attachment #774609 - Attachment is obsolete: true
Attachment #774606 - Attachment is obsolete: true
Attachment #774654 - Flags: review?(aki)
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.
Attachment #774654 - Flags: review?(aki) → review+
Thanks! Revised version of the patch.
Attachment #774654 - Attachment is obsolete: true
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
Attachment #775766 - Flags: review+
Attachment #775766 - Flags: checked-in+
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
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)
Attachment #775766 - Flags: checked-in+ → checked-in-
Depends on: 894353
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.)
Attachment #774651 - Attachment is obsolete: true
Attachment #775766 - Attachment is obsolete: true
Attachment #781686 - Flags: review?(aki)
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.
Attachment #781686 - Flags: review?(aki) → review+
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
Attachment #781686 - Flags: checked-in+
In production
See Also: → 892569
Product: mozilla.org → Release Engineering
Depends on: 931926
Depends on: 937637
Depends on: 949536
Depends on: 949538
We're now running from the test package and not from make check.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 994830
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.