Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12207172:
Change Android retry logic - always retry 3 times. (Closed)
|
Created:
7 years, 10 months ago by Isaac (away) Modified:
7 years, 10 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, frankf+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange Android retry logic - always retry 3 times.
Android retry logic currently changes based on number of
devices. This means that test runs are more flaky when only
one device is attached. Change this to always retry 3 times.
BUG=175653
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182420
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Messages
Total messages: 11 (0 generated)
https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... File build/android/pylib/base/base_test_sharder.py (right): https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... build/android/pylib/base/base_test_sharder.py:109: except android_commands.errors.DeviceUnresponsiveError as e: What happens if 4 devices are offline? It appears that no tests will run and the build will be green.
https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... File build/android/pylib/base/base_test_sharder.py (right): https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... build/android/pylib/base/base_test_sharder.py:109: except android_commands.errors.DeviceUnresponsiveError as e: On 2013/02/14 03:06:34, cjhopman wrote: > What happens if 4 devices are offline? It appears that no tests will run and the > build will be green. attached_devices is initially set to only the online devices. The question -- do more than 3 devices drop offline while pushing test binaries. I'd say we can just fail in that case.
https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... File build/android/pylib/base/base_test_sharder.py (right): https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... build/android/pylib/base/base_test_sharder.py:57: self.retries = 3 Do you have some data why the tot bot was flaky? Referring to the TODO above, was it device or test/product flakiness? The original reason for adding retries at all was to mitigate only device flakiness. There has been a lot of discussion (chromium-dev and offline) regarding not masking flakiness, but either fixing/disabling tests or fixing the product as the case may be.
https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... File build/android/pylib/base/base_test_sharder.py (right): https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... build/android/pylib/base/base_test_sharder.py:57: self.retries = 3 On 2013/02/14 03:42:15, frankf wrote: > Do you have some data why the tot bot was flaky? Referring to the TODO above, > was it device or test/product flakiness? The original reason for adding retries > at all was to mitigate only device flakiness. There has been a lot of discussion > (chromium-dev and offline) regarding not masking flakiness, but either > fixing/disabling tests or fixing the product as the case may be. I agree this code would be better if we distinguished between the two. But most bots have 3 or more devices, so they already are doing the retries. What I am trying to avoid here is different behavior based on number of phones attached. Currently the main builder masks flakiness because it has 7 phones.
lgtm, but this should be refactored soon. https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... File build/android/pylib/base/base_test_sharder.py (right): https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... build/android/pylib/base/base_test_sharder.py:57: self.retries = 3 Let's define it as a constant at the top of the module.
Thx for fast review. I suspect this logic is hiding a lot of test flakiness, not sure what to do about that... (see bug 175653) https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... File build/android/pylib/base/base_test_sharder.py (right): https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... build/android/pylib/base/base_test_sharder.py:57: self.retries = 3 On 2013/02/14 06:43:48, frankf wrote: > Let's define it as a constant at the top of the module. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ilevy@chromium.org/12207172/5002
Message was sent while issue was closed.
Change committed as 182420
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... File build/android/pylib/base/base_test_sharder.py (right): https://chromiumcodereview.appspot.com/12207172/diff/1/build/android/pylib/ba... build/android/pylib/base/base_test_sharder.py:109: except android_commands.errors.DeviceUnresponsiveError as e: On 2013/02/14 03:17:46, Isaac wrote: > On 2013/02/14 03:06:34, cjhopman wrote: > > What happens if 4 devices are offline? It appears that no tests will run and > the > > build will be green. > > attached_devices is initially set to only the online devices. The question -- > do more than 3 devices drop offline while pushing test binaries. I'd say we can > just fail in that case. I would be ok with just failing in that case, but it looks like we wouldn't fail, instead we would run no tests and pass. That behavior is much worse.
Message was sent while issue was closed.
https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... File build/android/pylib/base/base_test_sharder.py (right): https://codereview.chromium.org/12207172/diff/1/build/android/pylib/base/base... build/android/pylib/base/base_test_sharder.py:57: self.retries = 3 On 2013/02/14 06:53:10, Isaac wrote: > On 2013/02/14 06:43:48, frankf wrote: > > Let's define it as a constant at the top of the module. > > Done. It wasn't the bot that was flaky (it only had one device). The other bots just masked it. Min landed https://gerrit-int.chromium.org/#/c/32469/ which should fix one issue and 147905 can continue to track the flakiness. |
