New issue
Advanced search Search tips

Issue 918953 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

CQ build with odd behavior on Android -- requires investigation

Project Member Reported by erikc...@chromium.org, Jan 3

Issue description

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

We seem to be retrying tests unnecessarily, and then ignoring the results.

content_browsertests ('with patch') has 9 shards. shard #3 has a single failure, and shard #8 times out. This should cause 179 tests to be marked for retry.

content_browsertests ('without patch') runs 9 (?) shards, without any filtering (?) and all shard results are deduped. +thakis (yay? -- I think we do want deduping for 'without patch'). 

In 'retry with patch', we run all 9 shard again with no filtering. There is a flaky failure in shard #1. The build is marked as a success, presumably because all tests either passed in 'with patch' or 'retry with patch'.
 
Cc: martiniss@chromium.org
Here's what happened:

This CL is a refactor that merge duplicate logic between SwarmingIsolatedScriptTest and SwarmingGTestTest.

https://chromium-review.googlesource.com/c/chromium/tools/build/+/1361883/3/scripts/slave/recipe_modules/chromium_tests/steps.py#b1768

There used to be special case logic for SwarmingIsolatedScriptTest which ignores "test_to_retry" if there are over 100:
"""
    # We've run into issues in the past with command lines hitting a character
    # limit on windows. Do a sanity check, and only pass this list if we failed
    # less than 100 tests.
    if tests_to_retry and len(tests_to_retry) < 100:
      test_list = "::".join(tests_to_retry)
      args.extend(['--isolated-script-test-filter', test_list])
"""

Post refactor, the logic applied to both SwarmingIsolatedScriptTest and SwarmingGTestTest.

Given that we haven't been using this logic hasn't appeared to be necessary for SwarmingGtestTest, it seems surprising that we would need it for SwarmingIsolatedScriptTest. 

This logic was first added ~8 months ago, in the midst of a lot of lands/relands:
https://bugs.chromium.org/p/chromium/issues/detail?id=533481

I don't see a particular example where this caused issues...My first inclination is to just remove this logic and see if anything breaks. The other options would be to pass another flag so that we only use this logic for SwarmingIsolatedScriptTest. 

martiniss- WDYT?
I'd be fine removing this logic. I'd like to see if windows bots would break if there are too many tests though. We can test this with led and a test chromium CL which breaks a lot of tests on windows. I can remove this if you want, or you can. Up to you.
I'll take this one.
Test CL with 400+ failing tests in base_unittests:
https://ci.chromium.org/p/chromium/builders/luci.chromium.try/win7_chromium_rel_ng/161337

LED version of CL with (<100) limit removed:
https://ci.chromium.org/swarming/task/42337c565ef18910?server=chromium-swarm.appspot.com

Unfortunately, I think both are going to pass, even though this issue is still present. On Windows, the maximum command line length is 8191 characters:
https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation

On Linux/macOS, the limit is over 100,000.

Rather than having a 100-test limit, I think we should check that the post-concatenation length is within reasonable limits [e.g. < 7000 on Windows, <90,000 on other platforms].
"I think both are going to pass" -- what I meant to say is -- both will fail, but only in expected ways, not by hitting the command line max char limit.
The usual workaround for Windows's command line limit is to write the parameters or a file and then pass that as response file (`foo.exe @flags.txt`). Maybe the test launcher should support reading rsp files?
That does seem like the cleanest solution.  But it add complexity and require additional eng-time, which I don't think this problem currently merits.
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/a59cab1fc367487a170ffc2dfe5db62a1aabc97c

commit a59cab1fc367487a170ffc2dfe5db62a1aabc97c
Author: erikchen <erikchen@chromium.org>
Date: Mon Jan 07 16:18:21 2019

Use more accurate determination for maximum command line length.

Logic was added to SwarmingIsolatedScriptTest to prevent more than 100 tests
from being passed as a test filter, as this would sometimes cause the maximum
command line character limit to be exceeded on Windows.

A recent refactor unintentionally extended this logic to SwarmingGtestTest. This
was causing test filters to be skipped on non-Windows systems.

This CL changes the logic to instead skip the test filter only if it would
exceed reasonable lengths [7000 on Windows, 90000 on other platforms].

Bug: 918953
Change-Id: I63f90995ea8a45d17c9b9443c357edd7d05e3047
Reviewed-on: https://chromium-review.googlesource.com/c/1396041
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>

[modify] https://crrev.com/a59cab1fc367487a170ffc2dfe5db62a1aabc97c/scripts/slave/recipe_modules/chromium_tests/steps.py

Sign in to add a comment