Closed Bug 1014653 Opened 6 years ago Closed 5 years ago

Run C++ unit tests on B2G

Categories

(Testing :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: dminor)

References

Details

Attachments

(3 files)

We have C++ unit tests running on desktop and Android, but not B2G. There hasn't been any demand for this, but bug 867828 does want some WebRTC C++ unit tests running there, so this would be a prerequisite.
What do we mean by "B2G" here?  We're in the process of standing up Mulet B2G builds in buildbot, and we'll be running C++ tests against those.  Is that sufficient?
I suspect they actually want "on device or in emulator". Mulet is just going to be in a desktop environment, right? That's probably not sufficient for this.
yes, Mulet is just a desktop environment.  We'll have to run them on an emulator then, since we don't have an (easy) ability to run them on device.
I tried an emulator build today and we are already building and packaging the cppunit tests. The tests would run on an emulator using remotecppunittest.py over ADB: 20 passes, 51 failures. The failures appeared to be due to missing shared libraries, which required a bit of special handling on Android.

With luck, all we will need to get this going are some mozharness changes.
When running this through Mozharness with appropriate --localLib, I see 6 unexpected results. These seem to be the same tests we're currently skipping on Android along with TestPLDHash, which I believe we modified last time to get it running on Android.

We'll also need to add something to remotecppunittests.py to launch the emulator. Work is under way in bug 997244 will change how this is done, so I think it makes sense to wait on that bug for now.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Depends on: 997244
This adds support for starting the b2g emulator and adds the in-tree config files required to be able to run the tests from mozharness.
Attachment #8455377 - Flags: review?(gbrown)
Comment on attachment 8455377 [details] [diff] [review]
Add support for B2G emulator

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

Did you check that Android cppunittests via adb still work?

::: testing/b2g_cppunittest_manifest.txt
@@ +5,5 @@
> +TestNativeXMLHttpRequest            # Bug 919642
> +mediapipeline_unittest              # Bug 919646
> +TestStartupCache                    # Bug 929655
> +TestStartupCacheTelemetry.manifest  # Bug 929655
> +TestStartupCacheTelemetry.js        # Bug 929655

It seems odd that a manifest and js file are included here along with executables. I know that's the same as in the android manifest, but I wonder if it's necessary, and why.

::: testing/config/mozharness/b2g_emulator_config.py
@@ +52,5 @@
> +        "--dm_trans=adb",
> +        "--symbols-path=%(symbols_path)s",
> +        "--xre-path=%(xre_path)s",
> +        "--addEnv", "LD_LIBRARY_PATH=/vendor/lib:/system/lib:/system/b2g",
> +        "--withB2GEmulator=%(b2gpath)s",

--with-b2g-emulator maybe? There are so many different naming styles in use, I'm not sure what to suggest!

::: testing/remotecppunittests.py
@@ +249,5 @@
>          result = tester.run_tests(progs, options.xre_path, options.symbols_path)
>      except Exception, e:
>          log.error(str(e))
>          result = False
> +    if options.with_b2g_emulator and runner:

It looks like "and runner" may not be necessary here.
Attachment #8455377 - Flags: review?(gbrown) → review+
(In reply to Geoff Brown [:gbrown] from comment #7)
> Comment on attachment 8455377 [details] [diff] [review]
> Add support for B2G emulator
> 
> Review of attachment 8455377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you check that Android cppunittests via adb still work?
I just did, and it turns out some small changes are necessary to get things working. I need to specify a retryLimit explicitly rather than using None, and I need to see check if we're running with the emulator in the final except clause before trying to clean it up. Thanks for reminding me to check that.

> 
> ::: testing/b2g_cppunittest_manifest.txt
> @@ +5,5 @@
> > +TestNativeXMLHttpRequest            # Bug 919642
> > +mediapipeline_unittest              # Bug 919646
> > +TestStartupCache                    # Bug 929655
> > +TestStartupCacheTelemetry.manifest  # Bug 929655
> > +TestStartupCacheTelemetry.js        # Bug 929655
> 
> It seems odd that a manifest and js file are included here along with
> executables. I know that's the same as in the android manifest, but I wonder
> if it's necessary, and why.
The python script attempts to run everything in the directory, so it will push these to the device and try to run them, and then mark them as failed. This was the unsophisticated way of skipping these files.

> 
> ::: testing/config/mozharness/b2g_emulator_config.py
> @@ +52,5 @@
> > +        "--dm_trans=adb",
> > +        "--symbols-path=%(symbols_path)s",
> > +        "--xre-path=%(xre_path)s",
> > +        "--addEnv", "LD_LIBRARY_PATH=/vendor/lib:/system/lib:/system/b2g",
> > +        "--withB2GEmulator=%(b2gpath)s",
> 
> --with-b2g-emulator maybe? There are so many different naming styles in use,
> I'm not sure what to suggest!
I like that better too.

> 
> ::: testing/remotecppunittests.py
> @@ +249,5 @@
> >          result = tester.run_tests(progs, options.xre_path, options.symbols_path)
> >      except Exception, e:
> >          log.error(str(e))
> >          result = False
> > +    if options.with_b2g_emulator and runner:
> 
> It looks like "and runner" may not be necessary here.
Removed.

Thanks for the review!
Keywords: leave-open
Comment on attachment 8456844 [details] [diff] [review]
Add support for B2G emulator cppunit tests to Mozharness

lgtm.

I am not familiar with b2g_emulator_unittest.py so just a sanity check, I noticed we explicitly set test_manifest for some reftests[1] but IIUC, we rely on this[2] if condition a lot in preflight_run_tests. 

do we need to add a condition for cppunittest in that block? Can this here[3] be None in _query_abs_base_cmd?

[1] http://mxr.mozilla.org/build/source/buildbot-configs/mozilla-tests/b2g_config.py#726
[2] http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_emulator_unittest.py#316
[3] http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_emulator_unittest.py#270
Attachment #8456844 - Flags: review?(jlund) → review+
Thanks Jordan, good catch.

The 'test_manifest' at [1] can be None, because that is used to interpolate values in the in-tree mozharness config file (e.g. "%(test_manifest)s".)

Since the cppunit suite doesn't use the manifest file, it is ok that it isn't specified, and I did a quick check to make sure that it won't throw an exception if you interpolate something with a value of None.

To make it clear that test_manifest isn't needed for now, I'll add a default value of None to [2].

[1] http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_emulator_unittest.py#270
[2] http://mxr.mozilla.org/build/source/mozharness/scripts/b2g_emulator_unittest.py#316
> To make it clear that test_manifest isn't needed for now, I'll add a default
> value of None to [2].
> 

awesome. makes sense. tx :)
Comment on attachment 8456844 [details] [diff] [review]
Add support for B2G emulator cppunit tests to Mozharness

checked into production: http://hg.mozilla.org/build/mozharness/rev/2913947db89d
Depends on: 1045735
From a cedar run here [1] we are green on opt and have some unexpected results for debug:
TEST-UNEXPECTED-FAIL | ../../../gecko/storage/test/test_deadlock_detector.cpp | Sanity - deadlock not detected
TEST-UNEXPECTED-FAIL | ../../../gecko/storage/test/test_deadlock_detector.cpp | Sanity2 - deadlock not detected
TEST-UNEXPECTED-FAIL | ../../../gecko/storage/test/test_deadlock_detector.cpp | Sanity3 - deadlock not detected
TEST-UNEXPECTED-FAIL | ../../../gecko/storage/test/test_deadlock_detector.cpp | main - unknown child test
TEST-UNEXPECTED-FAIL | ../../../gecko/storage/test/test_deadlock_detector.cpp | ContentionNoDeadlock - deadlock
[1308] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cppremotecppunittests TEST-UNEXPECTED-FAIL | test_deadlock_detector | test failed with return code 1
[1404] WARNIremotecppunittests TEST-UNEXPECTED-FAIL | TestCSPParser | test failed with return code 139
remotecppunittests TEST-UNEXPECTED-FAIL | TestTArray | test failed with return code 137
TEST-UNEXPECTED-FAIL | ../../../gecko/xpcom/tests/TestDeadlockDetector.cpp | main - unknown child test
TEST-UNEXPECTED-FAIL | ../../../gecko/xpcom/tests/TestDeadlockDetector.cpp | ContentionNoDeadlock - deadlock
=remotecppunittests TEST-UNEXPECTED-FAIL | TestDeadlockDetector | test failed with return code 1
Return code: 1

[1] https://tbpl.mozilla.org/?tree=Cedar&rev=f3dceebdd32e
These are debug only failures. The manifest format doesn't permit these tests to be skipped on debug only so I have to skip them on opt as well. The deadlock tests don't appear to be built for opt anyway.

Cedar run here: https://tbpl.mozilla.org/?tree=Cedar&rev=dbcf32e278b5.
Attachment #8473663 - Flags: review?(gbrown)
Depends on: 1054255
Attachment #8473663 - Flags: review?(gbrown) → review+
Scheduled and running on trunk trees and aurora.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.