Open Bug 992323 Opened 10 years ago Updated 2 years ago

Get everything useful out of "make check"

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect

Tracking

(Not tracked)

People

(Reporter: ted, Unassigned)

References

(Depends on 1 open bug)

Details

"make check" sucks because:
1) It runs on build machines, using up cycles where we could be building instead
2) It doesn't run on test machines, where we actually test on the variety of operating systems that our users use
3) It doesn't run on Android or B2G devices
4) It doesn't run in cross-compile scenarios (if we were to fix bug 921040)
5) It runs recursively and wastes a lot of time trawling directories.
6) The test results can't be retriggered by sheriffs without rerunning the entire build.

We've made good progress in taking long-running things out of "make check": bug 949536 and bug 988532 took out the worst two offenders. There's still some cruft left though, so we should sort this out.

Here's my stated goals:
1) Move all the PYTHON_UNIT_TESTS to moz.build, add a target to run them non-recursively in one fell swoop. We'll continue running this after the build because these are build system unit tests that only make sense in this context.
2) Anything else running as part of make check should either be converted into a PYTHON_UNIT_TEST (if it's testing the build system) or punted out into another existing test suite (there are some random C++ tests that still get run) or removed entirely.
3) Stop running "make check" on the build machines.
Assignee: nobody → dminor
Bug 917363 is related.

I'd have to look at everything still in |make check|, but I don't think it's unreasonable to require everything to be part of a Python testing harness. Worst case, you have to create a Python shim file to invoke a command or something.

Also, it's not unreasonable for us to perform the Python tests between config.status and the build (perhaps only automatically in automation). Currently, sometimes we get all the way through a build only to find that we failed a unit test in python/mozbuild and the build is bad. But too late - we've already kicked off test jobs because |make check| runs after packaging. Moving the Python tests before the build should reduce automation costs.
One thing that is fishy is that we're currently linking gtest libxul during make check, because we didn't want to incur the cost of that linkage for people who don't want to run tests. We need to move that to the build but still keep that feature.
That's a good point, I forgot about that, we'll need to move gtests out of "make check" entirely, so we'll either want to do that gtest-libxul linking during package-tests, or controlled by a mozconfig option to limit it to the build slaves.
Depends on: 992983
Here's a list of check targets we have scattered about the tree:
http://mxr.mozilla.org/mozilla-central/search?string=^check%3A%3A&regexp=on&find=Makefile\.in%24&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

It looks like a lot of these could be moved into Python unittests.
Depends on: 994643
Depends on: 1003417
I'm not actively working on this, so I'm unassigning myself in case someone else is interested.
Assignee: dminor → nobody
Depends on: 1210759
Blocks: 927061
Blocks: 609125
ted, do you believe we should add this bug (stop running make check) to the list of bugs for the TaskCluster transition? (since we don't have sendchanges like in Buildbot). Otherwise, we're adding longer end to end times for pushes.
It's probably not unreasonable. If we fix bug 1210759 that should make the remainder of `make check` pretty quick. We will still have a few tests that need to get run after a build with a built objdir available, but they shouldn't take very long.
If possible to remove all tests it would be better for an issue related to the TaskCluster index.
If a task does not complete succesfully (e.g. there is a test failure), its artifacts won't become part of the index (perhaps we're OK with that). I'm going to make tools like mozci be able to find artifacts using the index.
Not making a task of the index would look to mozci as if the task did not upload any files.
Perhaps we can mark the task succesful yet the status for Treeherder is orange.
Perhaps I can make mozci find the artifacts for a push in a different way than using the index.
Blocks: 1243024
Blocks: 1080265
No longer blocks: 1243024
(In reply to Armen Zambrano [:armenzg] - Engineering productivity from comment #8)
> If possible to remove all tests it would be better for an issue related to
> the TaskCluster index.
> If a task does not complete succesfully (e.g. there is a test failure), its
> artifacts won't become part of the index (perhaps we're OK with that). I'm
> going to make tools like mozci be able to find artifacts using the index.

I don't think this is going to be feasible. We're going to want to have tests that check the resulting binaries in some way, but might not make sense to run as a standalone test suite.

> Not making a task of the index would look to mozci as if the task did not
> upload any files.
> Perhaps we can mark the task succesful yet the status for Treeherder is
> orange.

This sounds like the best option to me, if it's workable. "Succeeded with test failures" or something like that. I know for buildbot we had success/error/warnings, so warnings sounds like the best match.

> Perhaps I can make mozci find the artifacts for a push in a different way
> than using the index.

I don't think this is a good idea, the index seems really useful and I'd hate for that to get mucked up.
I'm removing this from bug 1080265 since it's not blocking migration to TC, and is instead an optimization.
No longer blocks: 1080265
I stand corrected -- this does block migration, specifically for OSX, where we are cross compiling and can't run make check.  Also, to support bug 1186848, we do need the "tests" (which should be orange on failure) to run in a different task than the builds (which turn red on failure).
In a little more detail, from an email from ted:

---

Most of what's in `make check` doesn't need the objdir. The largest thing remaining is PYTHON_UNIT_TESTS. Most of those could easily be run from a separate test job. There are a few of them that will have to be kept in the build job--they're checking properties of the built binaries. Those should be fine to run in cross-compiled builds. There might be a couple of weird things that need special handling, like testing/xpcshell/selftest.py, which runs tests on the xpcshell harness. That could be split off to be a special xpcshell test job variant. There are a few other random test binaries that run. We could probably just turn them off and nobody would care, they're old stuff that nobody has cared enough to migrate to a better testing environment.

If we got rid of everything except the few Python unit tests that want to test the resulting binaries, and made sure those were running in cross-compiled builds I think we'd be in good shape, and having those few tests turn the build red would not be that big of a deal.
For the few Python tests that do currently require a built objdir, we could probably split them out to a separate test task with a little work. We'd just have to make sure to package up anything they depend on from the objdir that's not already in a test package.
We should arguably run the Python build system tests *before* the build (or at least part of the build concurrent with something else) in automation so builds can fail fast if the build system itself isn't in a good state. By "Python build system tests" I mainly mean the unit tests in python/mozbuild and the random test scripts in build/ and config/.

As far as non-compiled tests go, my sense is mozbase accounts for the bulk of the wall time. I'd focus on moving those out first.
FWIW, this is blocking moving OS X tests to tier 1, which we need to do before the end of March.
Spoke with catlee and mshal (cc-ed) should be able to tackle this shortly.
Sure, I can work on this. Sounds like we have a good idea of what needs to be done.
Assignee: nobody → mshal
Status: NEW → ASSIGNED
Depends on: 1335796
Depends on: 1338608
Depends on: 1338415
See Also: → 1340698
Depends on: 1340699
Depends on: 1342230
Depends on: 1342233
Depends on: 1337903
I am not actively working on the remaining bugs here, instead focusing on the remaining blockers for getting the equivalent of full 'make check' coverage on cross OSX builds without actually enabling 'make check' there (bug 1340698).

I think it is still worthwhile to remove as much as we can out of 'make check' for the turnaround time wins, however.
Assignee: mshal → nobody
Status: ASSIGNED → NEW
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.