1204662 - running cpp tests from manifests leads to unexpectedly broken code and/or tests

Status: NEW

(Testing :: CPPUnitTest, defect)

Description

[Nathan Froyd [:froydnj]]

We have a list of C++ unit tests in MFBT:

http://mxr.mozilla.org/mozilla-central/source/mfbt/tests/moz.build#7

In one recent bug, we found that tests added to that list after C++ tests were run solely from manifests were not being run...which was bad, because the tests that had been added were broken!

In code I reviewed today, somebody had added their test to the above list...I'm guessing it was green on a try run or on their local machine, but the test itself was broken.  And the patch author had not detected that, because the test wasn't getting run.

We need to fix this, either by changing the build config to understand how to build tests from manifests, or by changing the way C++ tests get run to consider the authoritative source of information the build system, rather than manifests.

Comment 1

[Dan Minor [:dminor]]

I added the manifests when getting the C++ unit tests running on Android and B2G because I thought it would be easier to edit a text file rather than edit the build configuration to enable and disable tests which I in turn hoped would make it more likely that the broken tests on these platforms get fixed.  It seemed strange to me that we used manifests to skip tests for our other test frameworks but not for the C++ tests.

I'm sorry if this created confusion and I certainly have no objection to going back to just building the tests you want to run for a given platform.

Comment 2

[(not currently active) Ted Mielczarek]

I mentioned this in bug 1190474 comment 2, it's definitely less than great.

(In reply to Nathan Froyd [:froydnj] from comment #0)
> We need to fix this, either by changing the build config to understand how
> to build tests from manifests, or by changing the way C++ tests get run to
> consider the authoritative source of information the build system, rather
> than manifests.

One way to do this would be to make CPP_UNIT_TESTS a StrictOrderingOnAppendListWithFlagsFactory (that's a mouthful) like SOURCES, so we could stick manifest annotations for them in moz.build, like maybe (per the existing moz.build/manifest):
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/dom/media/compiledtest/moz.build#8
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/testing/cppunittest.ini?offset=0#6
```
GeckoCppUnitTests([
    'TestAudioBuffers',
    ...
])

CPP_UNIT_TESTS['TestAudioBuffers']['skip-if'] = "os == 'b2g'"
```

...not beautiful, but could be made to work.