I think that a more reliable implementation would be to run:

  equery-${BOARD} hasuse test

This only returns packages that have tests and doesn't require that the package declared IUSE=test (as no package should). Aviv/Mike, what do you think?

you've lost me.  IUSE=test is set in an ebuild only if it has a conditional dependency related to its test usage.  it makes no statement wrt the ebuild itself supporting tests.

that's why we've been requiring an explicit src_test function.  it communicates "the ebuild has a valid test function that can be run".

if you're looking at platform2 packages, the reason those have IUSE=test is because the platform eclass sets it, and it does so to provide consistent test package deps (gtest/gmock).  it does not mean every platform package supports tests (in fact, some don't, like u2fd).  so if we based it on the USE flag, it'd mean we build more packages than necessary just to have them run tests that don't exist.

the exact logic we're talking about here:

scripts/cros_run_unit_tests.py:main
  pkg_with_test = portage_util.PackagesWithTest(sysroot, packages)

lib/portage_util.py:PackagesWithTest
  pkg_with_test = set(parallel.RunTasksInProcessPool(_CheckHasTest, inputs))

lib/portage_util.py:_CheckHasTest
  ebuild = EBuild(path)
  return cp if ebuild.has_test else None

lib/portage_util.py:EBuild.Classify
    for line in fileinput.input(ebuild_path):
      elif (line.startswith('src_test()') or
            line.startswith('platform_pkg_test()')):
        has_test = True

I'm happy to be wrong: thanks for the background and the additional data on packages this wouldn't work for. So, I agree we should scrap my idea in #c1.

> we've been requiring an explicit src_test

The problem is that this requirement isn't documented and that CBuildBot behaves differently than "FEATURES=test emerge package" does within our own CrOS SDK environment.

I'd gladly add the documentation for the requirement if we make Portage behave the same as CBuildBot.

Or, we can fix CBuildBot to match Portage (what this bug was opened to track). Is there some other way that we can poke at packages to extract whether they would execute some local or inherited src_test when FEATURES=test is enabled? I poked around with equery has a bit but couldn't find a KEY name that seemed to reveal this.

What do you think?

to back up a bit more, the reason for the current behavior isn't a bug or laziness.  we need to strike a balance with only testing packages that we care about (and not running tests for every single package in the depgraph), and we need to not waste time building large packages that don't have any tests (arguably not a situation we want, but it is what it is, and it's not changing).  we've opted to go with:
- if the package is a cros-workon package, it's something we're generally "working on" vs a random 3rd party package we're taking straight as a release
- if the ebuild defines a src_test, that's a clear signal it has a test phase that we want to run

with that in mind, what you want to do is support our cros-workon packages that have a test phase defined via an eclass only and not add an ebuild-specific stub.  that might be doable, but we need data first to see what changes.

the way Gentoo PMS works is that it provides a default src_test that probes to see if certain common methods for testing exist (like `make check`), and if so, runs them, and if not, just skips the phase.  thus the default case doesn't provide anything you can query to answer the question "does this package have a test".

if we look at it in terms of ebuilds that explicitly define src_test, either directly or indirectly via eclasses, we can do that with the metadata.
  $ git grep 'DEFINED_PHASES=.*\<test\>' metadata/md5-cache/

i don't think we have any tooling today, either in portage or in chromite, that allows cli queries.  we'd have to build something ourselves.  the format is pretty simple though, so that shouldn't be a big deal, and we've had other requests where people want to build off of this metadata.

looking at amd64-generic-full, these packages are currently being skipped, but have a src_test phase defined via an eclass:
chromeos-base/chromeos-firmware-null    cros-firmware       the one package you guys want
chromeos-base/container_utils           platform            doesn't have tests, but prob should
chromeos-base/debugd-client             platform            duplicates debugd
chromeos-base/ec-devutils               distutils-r1        don't think it has any tests
chromeos-base/permission_broker-client  platform            duplicates permission_broker
chromeos-base/power_manager-client      platform            duplicates power_manager
chromeos-base/secure-erase-file         platform            no actual tests, prob should
chromeos-base/session_manager-client    platform            duplicates chromeos-login
chromeos-base/shill-client              platform            duplicates shill
chromeos-base/system_api                platform            no actual tests, prob should
chromeos-base/tast-common               cros-go             not sure how useful go tests are
chromeos-base/tast-local-tests          cros-go             not sure how useful go tests are
chromeos-base/touch_firmware_test       distutils-r1        don't think it has any actual tests
chromeos-base/update_engine-client      platform            duplicates update_engine
dev-util/hdctools                       distutils-r1        has some tests, but don't think the ebuild triggers it
sys-kernel/chromeos-kernel-4_4          cros-kernel2        has a test we prob want
x11-libs/libdrm                         autotools-multilib  has tests?

so if we were to run all the packages that have an explicit src_test, that's a decent chunk of wasted time.  there's already large concerns with increasing CQ turn around time.

we could support RESTRICT=test as a way of ebuilds opting into "don't test me".  binutils today is done by hardcoding it in lib/portage_util.py, but that sucks.

Thanks, Mike. The comprehensive list of packages in #c6 is compelling evidence that we can manage this via a nuanced approach vs my large proposal in #c5 to make Portage and CbuildBot behavior equal.

So, to implement a nuanced approach, what about a presubmit check that looks for test phases defined in eclasses but not activated in the inherited ebuild? Ebuilds that don't want to inherit from the eclass (such as the duplicate cases) would be able to opt out of this check with RESTRICT=test.

Then, I can go through the list and submit some changes to activate the src_test phases that we think were intended to be run but currently aren't.

Good/bad idea?

i'm not sure what you mean by the preupload hook.  you want to make ebuild authors explicitly choose either RESTRICT="test" or declare an explicit src_test line ?  i'd be fine with that policy for all cros-workon packages (since that's what our unittest stage looks at currently).  it would be a bit more pain up front, but would probably be worth it long term.

Yes, that's what I mean. Taking this and planning to implement that, then.