New issue
Advanced search Search tips

Issue 900977 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

network_service_content_browsertests flakily failed with test runner error

Project Member Reported by erikc...@chromium.org, Nov 1

Issue description

task: https://chromium-swarm.appspot.com/task?id=40e40f67d9921610&refresh=10&show_raw=1

CL: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/115389

"""
test_runner.py gtest: error: argument -f/--test-filter/--gtest_filter/--gtest-filter: not allowed with argument --gtest-filter-file/--test-launcher-filter-file
"""
 
"""
Command: /b/swarming/w/ir/.swarming_module_cache/vpython/f22999/bin/python ../../build/android/test_wrapper/logdog_wrapper.py --target content_browsertests --logdog-bin-cmd ../../bin/logdog_butler --store-tombstones --enable-features=NetworkService --test-launcher-filter-file=../../testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter --gs-results-bucket=chromium-result-details --recover-devices --gtest_repeat=10 --gtest_filter=SignedExchangeRequestHandlerBrowserTest/SignedExchangeRequestHandlerBrowserTest.Expired/0 --test-launcher-summary-output=/b/swarming/w/iodO1YbP/output.json
"""

Relevant lines:
--test-launcher-filter-file=../../testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
--gtest_filter=SignedExchangeRequestHandlerBrowserTest/SignedExchangeRequestHandlerBrowserTest.Expired/0

The latter is set by the 'retry with patch' step, as we're trying to rerun failing tests. 
The former comes from https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?type=cs&q=network_content_browsertests&g=0&l=1988.
This is that same CL :/
Why are these mutually exclusive? Is there a particular reason? I think they both should work, or we could somehow remove the filter file argument on the retry.
I see many more examples of "test-launcher-filter-file" in test_suites.pyl. 

I would expect any 'retry with patch' or 'retry without patch' step to fail when run on a test suite that also sets "test-launcher-filter-file".
Just because muxing them in https://codesearch.chromium.org/chromium/src/build/android/pylib/utils/test_filter.py?rcl=e52da1b8552c9a23e332dfc63b5cb480f3c6966f&l=69 isn't implemented; now that it's necessary, there's no particular reason not to implement it.
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
I think that --gtest_filter should override --test-launcher-filter-file, which would also be very easy to implement. Objections?
#6 SGTM
That doesn't seem particularly intuitive?
If a user [or script] is specifying --gtest_filter, then they're indicating that they want a very specific suite of tests to run.

On the other hand, --test-launcher-filter-file appears to be used to generically filter out a large number of tests.

Most of the time, I expect there to be no conflict between --gtest_filter and --test-launcher-filter-file. E.g. in the case of recipes, in order for a test to fail, it must not have been filtered by test-launcher-filter-file.

If for some reason there is a conflict, then preferring "gtest_filter" seems reasonable since it has higher specificity, and is a whitelist rather than a blacklist.

If a user is specifying both, I don't see why we'd have reason to think one is preferred over the other. (I do get the recipe case.)

I suspect, given how the recipes use the filter flag, that we instead want to teach the scripts how to merge filters and then support merging if multiple --gtest_filters are provided or if --gtest_filter and --test-launcher-filter-file are provided.
You have more context than me on the best approach here. 

I generally prefer the simplest approach that will solve the current problems that we're seeing, but if you think that filter merging is the right approach, then I'll implement that instead.
hrm. we don't really want the sum of the two filters per say; more like two independent filters... but implementing that involves basically rewriting the filter system in the android test runner, which seems a bit much.

I'm wary of surprising people, but perhaps preferring --gtest_filter in the presence of both is the right course of action. (or perhaps the recipe should fiddle with the command-line when it adds --gtest_filter?)
> or perhaps the recipe should fiddle with the command-line when it adds --gtest_filter

This is currently tricky to do robustly because test launcher filters are currently added as config strings. We'd need to parse the strings and operate on them, which I'd like to avoid if possible.

> I'm wary of surprising people, but perhaps preferring --gtest_filter in the presence of both is the right course of action.

We could emit a warning message? That way recipes will work fine, and if a user is trying to run things locally they'll get a warning.

Sign in to add a comment