I had advocated in ~2010 for test_launcher to fail if there were no test run but got overruled. I don't think it's infra's duty: the task passed.

Tests have wildly varying runtimes in some cases (e.g. asan), which means there would be a significant number of edge cases to handle. Furthermore, the same test binary is used in different forms, with flag creating wildly different runtime characteristics; mash being a prime example. The infra wouldn't be tooled to diagnose these on a continuous basis.

Ref: https://cs.chromium.org/chromium/src/base/test/launcher/

Yeah, I see how timing could vary a lot. So that wasn't a good idea on my part. :-)

Do you remember who overruled your argument about test_launcher and/or why? I've now got real-world examples of a team getting burned by this.

See also https://chromium-review.googlesource.com/c/chromium/src/+/1144053 where I forgot to add a '#' to a comment in the filters. Thanks to the fact that I saw this bug fly by, I just now realized that the test isn't actually doing anything:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8939911533006501680/+/steps/chromeos_unittests/0/stdout

*facepalm*

The least we can do is modify //testing/buildbot/filter/'s presubmit to enforce a known-good format for the filter. I can do that.

Also +ICC again since we own most everything there.

See https://chromium-review.googlesource.com/c/chromium/src/+/923552 for discussion about format validation, we might be able to revert that CL if we had a better presubmit.

re changing //base/test/launcher: I agree with the sentiment, but even "bot didn't run anything" shouldn't necessarily be a failure. e.g., the w/o patch retries pass a filter consisting of only tests that failed w/ the patch, and if a new test is the only thing that failed, you'd incorrectly have the step fail.

(though I was not the person who disagreed in 2010)

+sky/dpranke who were involved in these issues before.

I'll leave it to you guys to decide what's best to do here. Summary of the historical mistakes I've seen in filter files:

### // isn't a valid comment so it adds a positive rule, then nothing is selected.
// TODO: Fix this
-Foo.Bar

### Adds test TODO, which doesn't exist, so nothing runs
TODO: Fix this
-Foo.Bar

### Trying to add a couple positive exceptions to a big blacklist
-CommonPrefix.*
CommonPrefix.ThisOneWorks
-OtherPrefix.*

I wonder if tweaking test_launcher would be OK if the logic was something like:
* Did you specify a filter file, and not --gtest_filter?
* If so, but no tests remain after the filter, then fail
Then devs/bots could do whatever they want with --gtest_filter.

Either way, presubmit seems valuable, especially if it could catch the 3rd case.

I think in the normal case, running zero tests should be an error.

jbudorick@ is correct that in the "without patch" case you may end up with no tests being run as a correct thing. You may also see this when running things under FindIt. 

We tripped over the last case at one point recently with the layout tests and added a --zero-tests-executed-ok flag to make that clear. I think such a flag should be part of the generic contract for test steps, and if we have that, then zero tests should be an error by default, the switch should be added in the cases where we expect this might happen, and we can also support the switch for whatever weird edge cases we trip over.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b63e6608c4ee51a8b9abdfc2ff19f8b869228f08

commit b63e6608c4ee51a8b9abdfc2ff19f8b869228f08
Author: Ben Pastene <bpastene@chromium.org>
Date: Fri Jul 27 21:03:10 2018

Add a presubmit check to ensure sytnax of test filters files is valid.

This prints an error if:
- A line begins with '//' or '/*'
- A non-comment contains a whitespace
And prints a warning if:
- A filter file contains both inclusions and exclusions

Bug: 868000
Change-Id: I2a04504d3a17f4f7536b529a0e2e626eb6e61f66
Reviewed-on: https://chromium-review.googlesource.com/1153427
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578784}
[modify] https://crrev.com/b63e6608c4ee51a8b9abdfc2ff19f8b869228f08/testing/buildbot/filters/PRESUBMIT.py
[modify] https://crrev.com/b63e6608c4ee51a8b9abdfc2ff19f8b869228f08/testing/buildbot/filters/README.md
[modify] https://crrev.com/b63e6608c4ee51a8b9abdfc2ff19f8b869228f08/testing/buildbot/filters/chromeos.chromeos_unittests.filter
[modify] https://crrev.com/b63e6608c4ee51a8b9abdfc2ff19f8b869228f08/testing/legion/lib/rpc/jsonrpclib.py