Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1364953003:
Android: Change test timeout to 30 minutes instead of 15. (Closed)
|
Created:
5 years, 2 months ago by kjellander_chromium Modified:
5 years, 2 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
http://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid: Change test timeout to 30 minutes instead of 15.
Before the recent changes in how Android tests are run, tests with
runtimes around 22 minutes were executing fine. Somewhere in the range
of 310ea93..6323fa9 (349094:350405) this has changed, which breaks
some WebRTC tests that run on a single device (no sharding).
See https://codereview.webrtc.org/1355083002/ for details.
Increasing the timeout fixes this for now, but ideally it would be
configurable instead (future improvement).
BUG=535973
Committed: https://crrev.com/2372f179997d65642e3ada0bf05cfca9d3887f36
Cr-Commit-Position: refs/heads/master@{#351054}
Patch Set 1 #
Messages
Total messages: 20 (5 generated)
kjellander@chromium.org changed reviewers: + jbudorick@chromium.org, pasko@chromium.org
This would enable WebRTC perf tests to keep passing and unblock our rolling. pasko@: if you're comfortable letting this go in quickly. jbudorick@: as we've chatted offline about this.
The CQ bit was checked by kjellander@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1364953003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1364953003/1
what is the typical time to run these tests before and now? did anything change with the way stdout is grabbed in tests? (buildbot kills the step if no output shows in X minutes, where X depends on the bot, but is around 15-20 minutes afair) please put BUG=something for context how urgent is this?
On 2015/09/25 12:57:33, pasko wrote: > what is the typical time to run these tests before and now? > did anything change with the way stdout is grabbed in tests? (buildbot kills the > step if no output shows in X minutes, where X depends on the bot, but is around > 15-20 minutes afair) Our webrtc_perf_tests have always been around 22-24 minutes. Example build: http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... > please put BUG=something for context Done. > how urgent is this? Not critical, but the next few days as we're about 2 weeks behind in rolling now, which slows down some parts of the team.
On 2015/09/25 at 13:11:42, kjellander wrote: > On 2015/09/25 12:57:33, pasko wrote: > > what is the typical time to run these tests before and now? > > did anything change with the way stdout is grabbed in tests? (buildbot kills the > > step if no output shows in X minutes, where X depends on the bot, but is around > > 15-20 minutes afair) > > Our webrtc_perf_tests have always been around 22-24 minutes. Example build: http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... I don't actually know how these tests previously cleared the old 20-minute infra timeout, then. > > > please put BUG=something for context > > Done. > > > how urgent is this? > > Not critical, but the next few days as we're about 2 weeks behind in rolling now, which slows down some parts of the team.
> > how urgent is this? > > Not critical, but the next few days as we're about 2 weeks behind in rolling > now, which slows down some parts of the team. That's unfortunate. I think we can land this timeout change now if phajdan.jr@ confirms that this won't make CQ unhappy (I am not an expert here, but assume that CQ may not have its own timeouts, which would make it slower to run bad tries). This is purely hypothetical, and will probably have a response quickly. > > what is the typical time to run these tests before and now? > > did anything change with the way stdout is grabbed in tests? (buildbot kills > the > > step if no output shows in X minutes, where X depends on the bot, but is > around > > 15-20 minutes afair) > > Our webrtc_perf_tests have always been around 22-24 minutes. Example build: > http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... In your local testing, did the output become shorter than the one linked above? If so, my hypothesis about killing-without-output suggests that this change won't help. If the output is roughly the same length, this change lgtm with approval from phajdan.jr@
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/25 13:17:09, jbudorick wrote: > On 2015/09/25 at 13:11:42, kjellander wrote: > > On 2015/09/25 12:57:33, pasko wrote: > > > what is the typical time to run these tests before and now? > > > did anything change with the way stdout is grabbed in tests? (buildbot kills > the > > > step if no output shows in X minutes, where X depends on the bot, but is > around > > > 15-20 minutes afair) > > > > Our webrtc_perf_tests have always been around 22-24 minutes. Example build: > http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... > > I don't actually know how these tests previously cleared the old 20-minute infra > timeout, then. The 20 minute infra timeout only applies if the test isn't printing anything to stdout for 20 minutes. As long as it does that, that timer is reset. > > > > > > please put BUG=something for context > > > > Done. > > > > > how urgent is this? > > > > Not critical, but the next few days as we're about 2 weeks behind in rolling > now, which slows down some parts of the team.
On 2015/09/25 at 17:11:58, kjellander wrote: > On 2015/09/25 13:17:09, jbudorick wrote: > > On 2015/09/25 at 13:11:42, kjellander wrote: > > > On 2015/09/25 12:57:33, pasko wrote: > > > > what is the typical time to run these tests before and now? > > > > did anything change with the way stdout is grabbed in tests? (buildbot kills > > the > > > > step if no output shows in X minutes, where X depends on the bot, but is > > around > > > > 15-20 minutes afair) > > > > > > Our webrtc_perf_tests have always been around 22-24 minutes. Example build: > > http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... > > > > I don't actually know how these tests previously cleared the old 20-minute infra > > timeout, then. > > The 20 minute infra timeout only applies if the test isn't printing anything to stdout for 20 minutes. As long as it does that, that timer is reset. That's from the bot side. The previous test runner code would hard-cap timeouts at 20 minutes to avoid the buildbot one. (I'd add a codesearch link, but I deleted the code in question this week.) > > > > > > > > > > please put BUG=something for context > > > > > > Done. > > > > > > > how urgent is this? > > > > > > Not critical, but the next few days as we're about 2 weeks behind in rolling > > now, which slows down some parts of the team.
On 2015/09/25 13:25:15, pasko wrote: > > > how urgent is this? > > > > Not critical, but the next few days as we're about 2 weeks behind in rolling > > now, which slows down some parts of the team. > > That's unfortunate. > > I think we can land this timeout change now if phajdan.jr@ confirms that this > won't make CQ unhappy (I am not an expert here, but assume that CQ may not have > its own timeouts, which would make it slower to run bad tries). This is purely > hypothetical, and will probably have a response quickly. > > > > what is the typical time to run these tests before and now? > > > did anything change with the way stdout is grabbed in tests? (buildbot kills > > the > > > step if no output shows in X minutes, where X depends on the bot, but is > > around > > > 15-20 minutes afair) > > > > Our webrtc_perf_tests have always been around 22-24 minutes. Example build: > > > http://build.chromium.org/p/client.webrtc/builders/Android32%20Tests%20%28L%2... > > In your local testing, did the output become shorter than the one linked above? > If so, my hypothesis about killing-without-output suggests that this change > won't help. > > If the output is roughly the same length, this change lgtm with approval from > phajdan.jr@ My local run with an extended timeout was along the lines of 20 minutes too (I didn't keep the logs though). Since this timeout of 15 minutes for the test executable kills the test no matter if it's printing to stdout, the infra killing-without-output doesn't have any effect here (and it also shows up as differently in the bot output, with a TIMEOUT at the end of the log). During my try runs, some tests passed and the long-running ones ended up with UNKNOWN result.
kjellander@chromium.org changed reviewers: + phajdan.jr@chromium.org
Actually adding Pawel to the review. Please check pasko and jbudorick's comments above where they ask if this could affect the CQ.
LGTM
The CQ bit was checked by kjellander@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1364953003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1364953003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2372f179997d65642e3ada0bf05cfca9d3887f36 Cr-Commit-Position: refs/heads/master@{#351054} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
