Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897183003:
[chromecast] Enables cma tests if is_cast_using_cma_backend.
Description[chromecast] Enables cma tests if is_cast_using_cma_backend.
Patch Set 1 #Patch Set 2 : shard timeout #Patch Set 3 : is android #Messages
Total messages: 26 (10 generated)
The CQ bit was checked by alokp@chromium.org to run a CQ dry run
alokp@chromium.org changed reviewers: + tsunghung@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/23 23:35:13, alokp wrote: Andy: Could you also please test the patch since I do not think this config is currently tested by CQ?
halliwell@chromium.org changed reviewers: + thoren@chromium.org
On 2017/05/24 04:22:00, alokp wrote: > On 2017/05/23 23:35:13, alokp wrote: > > Andy: Could you also please test the patch since I do not think this config is > currently tested by CQ? Thoren probably has more time to test this: could you run these tests on a Things device?
On 2017/05/24 15:24:10, halliwell wrote: > On 2017/05/24 04:22:00, alokp wrote: > > On 2017/05/23 23:35:13, alokp wrote: > > > > Andy: Could you also please test the patch since I do not think this config is > > currently tested by CQ? > > Thoren probably has more time to test this: could you run these tests on a > Things device? All of them pass except for Required/MultizoneBackendTest.RenderingDelay/33 It doesn't fail, but times out. Relaxing the timeout to 300s lets all tests pass.
On 2017/05/24 17:51:34, thoren wrote: > On 2017/05/24 15:24:10, halliwell wrote: > > On 2017/05/24 04:22:00, alokp wrote: > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > Andy: Could you also please test the patch since I do not think this config > is > > > currently tested by CQ? > > > > Thoren probably has more time to test this: could you run these tests on a > > Things device? > > All of them pass except for Required/MultizoneBackendTest.RenderingDelay/33 > It doesn't fail, but times out. Relaxing the timeout to 300s lets all tests > pass. Hmm. Which step is slow?
On 2017/05/24 18:00:00, alokp wrote: > On 2017/05/24 17:51:34, thoren wrote: > > On 2017/05/24 15:24:10, halliwell wrote: > > > On 2017/05/24 04:22:00, alokp wrote: > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > Andy: Could you also please test the patch since I do not think this > config > > is > > > > currently tested by CQ? > > > > > > Thoren probably has more time to test this: could you run these tests on a > > > Things device? > > > > All of them pass except for Required/MultizoneBackendTest.RenderingDelay/33 > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all tests > > pass. > > Hmm. Which step is slow? Each test takes around 2-4s, but the number of them in that group causes the command running the whole group to time out consistently at number 33. If you filter out earlier tests it will time out on a different one. I was running with --fast-local-dev, which sets num-retries to 0. If you allow retries, they all work. Whether this is working as intended, I can't say.
On 2017/05/24 21:32:39, thoren wrote: > On 2017/05/24 18:00:00, alokp wrote: > > On 2017/05/24 17:51:34, thoren wrote: > > > On 2017/05/24 15:24:10, halliwell wrote: > > > > On 2017/05/24 04:22:00, alokp wrote: > > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > > > Andy: Could you also please test the patch since I do not think this > > config > > > is > > > > > currently tested by CQ? > > > > > > > > Thoren probably has more time to test this: could you run these tests on a > > > > Things device? > > > > > > All of them pass except for Required/MultizoneBackendTest.RenderingDelay/33 > > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all tests > > > pass. > > > > Hmm. Which step is slow? > > Each test takes around 2-4s, but the number of them in that group causes the > command running the whole group to time out consistently at number 33. If you > filter out earlier tests it will time out on a different one. I was running with > --fast-local-dev, which sets num-retries to 0. If you allow retries, they all > work. > > Whether this is working as intended, I can't say. Which timeout are we talking about here?
On 2017/05/24 21:32:39, thoren wrote: > On 2017/05/24 18:00:00, alokp wrote: > > On 2017/05/24 17:51:34, thoren wrote: > > > On 2017/05/24 15:24:10, halliwell wrote: > > > > On 2017/05/24 04:22:00, alokp wrote: > > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > > > Andy: Could you also please test the patch since I do not think this > > config > > > is > > > > > currently tested by CQ? > > > > > > > > Thoren probably has more time to test this: could you run these tests on a > > > > Things device? > > > > > > All of them pass except for Required/MultizoneBackendTest.RenderingDelay/33 > > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all tests > > > pass. > > > > Hmm. Which step is slow? > > Each test takes around 2-4s, but the number of them in that group causes the > command running the whole group to time out consistently at number 33. If you > filter out earlier tests it will time out on a different one. I was running with > --fast-local-dev, which sets num-retries to 0. If you allow retries, they all > work. > > Whether this is working as intended, I can't say. I thought each test instance gets 30s. Timeout for a test group does not make sense because it will depend on the number of tests in the group, which can be arbitrarily large. I also did not see this 300s timeout interval. Can you point me to where you are changing this number. It sounds like we have a bug in MultizoneBackendTest or Things cma backend.
On 2017/05/24 22:41:34, alokp wrote: > On 2017/05/24 21:32:39, thoren wrote: > > On 2017/05/24 18:00:00, alokp wrote: > > > On 2017/05/24 17:51:34, thoren wrote: > > > > On 2017/05/24 15:24:10, halliwell wrote: > > > > > On 2017/05/24 04:22:00, alokp wrote: > > > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > > > > > Andy: Could you also please test the patch since I do not think this > > > config > > > > is > > > > > > currently tested by CQ? > > > > > > > > > > Thoren probably has more time to test this: could you run these tests on > a > > > > > Things device? > > > > > > > > All of them pass except for > Required/MultizoneBackendTest.RenderingDelay/33 > > > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all > tests > > > > pass. > > > > > > Hmm. Which step is slow? > > > > Each test takes around 2-4s, but the number of them in that group causes the > > command running the whole group to time out consistently at number 33. If you > > filter out earlier tests it will time out on a different one. I was running > with > > --fast-local-dev, which sets num-retries to 0. If you allow retries, they all > > work. > > > > Whether this is working as intended, I can't say. > > I thought each test instance gets 30s. Timeout for a test group does not make > sense because it will depend on the number of tests in the group, which can be > arbitrarily large. I also did not see this 300s timeout interval. Can you point > me to where you are changing this number. It sounds like we have a bug in > MultizoneBackendTest or Things cma backend. It's a parameter called "-t SHARD_TIMEOUT" for the test running script generated when you build a test target. (at out*/bin/run_cast_media_unittests). I don't fully understand how it works, but it seems like this script splits the tests up somehow, and executes the adb commands necessary to run them on the device. The timeout parameter for the script appears to apply to the adb commands, so my guess is that gUnit is working properly, but the script wrapping it doesn't expect the command to take so long.
On 2017/05/24 22:41:34, alokp wrote: > On 2017/05/24 21:32:39, thoren wrote: > > On 2017/05/24 18:00:00, alokp wrote: > > > On 2017/05/24 17:51:34, thoren wrote: > > > > On 2017/05/24 15:24:10, halliwell wrote: > > > > > On 2017/05/24 04:22:00, alokp wrote: > > > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > > > > > Andy: Could you also please test the patch since I do not think this > > > config > > > > is > > > > > > currently tested by CQ? > > > > > > > > > > Thoren probably has more time to test this: could you run these tests on > a > > > > > Things device? > > > > > > > > All of them pass except for > Required/MultizoneBackendTest.RenderingDelay/33 > > > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all > tests > > > > pass. > > > > > > Hmm. Which step is slow? > > > > Each test takes around 2-4s, but the number of them in that group causes the > > command running the whole group to time out consistently at number 33. If you > > filter out earlier tests it will time out on a different one. I was running > with > > --fast-local-dev, which sets num-retries to 0. If you allow retries, they all > > work. > > > > Whether this is working as intended, I can't say. > > I thought each test instance gets 30s. Timeout for a test group does not make > sense because it will depend on the number of tests in the group, which can be > arbitrarily large. I also did not see this 300s timeout interval. Can you point > me to where you are changing this number. It sounds like we have a bug in > MultizoneBackendTest or Things cma backend. Alok: seems like it's shard_timeout in the Android test runner. It's 120s by default (and I notice that some other tests override the shard_timeout in gn). I didn't see yet exactly what group of tests is grouped in this shard timeout.
On 2017/05/24 23:22:01, halliwell wrote: > On 2017/05/24 22:41:34, alokp wrote: > > On 2017/05/24 21:32:39, thoren wrote: > > > On 2017/05/24 18:00:00, alokp wrote: > > > > On 2017/05/24 17:51:34, thoren wrote: > > > > > On 2017/05/24 15:24:10, halliwell wrote: > > > > > > On 2017/05/24 04:22:00, alokp wrote: > > > > > > > On 2017/05/23 23:35:13, alokp wrote: > > > > > > > > > > > > > > Andy: Could you also please test the patch since I do not think this > > > > config > > > > > is > > > > > > > currently tested by CQ? > > > > > > > > > > > > Thoren probably has more time to test this: could you run these tests > on > > a > > > > > > Things device? > > > > > > > > > > All of them pass except for > > Required/MultizoneBackendTest.RenderingDelay/33 > > > > > It doesn't fail, but times out. Relaxing the timeout to 300s lets all > > tests > > > > > pass. > > > > > > > > Hmm. Which step is slow? > > > > > > Each test takes around 2-4s, but the number of them in that group causes the > > > command running the whole group to time out consistently at number 33. If > you > > > filter out earlier tests it will time out on a different one. I was running > > with > > > --fast-local-dev, which sets num-retries to 0. If you allow retries, they > all > > > work. > > > > > > Whether this is working as intended, I can't say. > > > > I thought each test instance gets 30s. Timeout for a test group does not make > > sense because it will depend on the number of tests in the group, which can be > > arbitrarily large. I also did not see this 300s timeout interval. Can you > point > > me to where you are changing this number. It sounds like we have a bug in > > MultizoneBackendTest or Things cma backend. > > Alok: seems like it's shard_timeout in the Android test runner. It's 120s by > default (and I notice that some other tests override the shard_timeout in gn). > I didn't see yet exactly what group of tests is grouped in this shard timeout. Seems like there are a significant number of tests run in the shard because of the parametric combinations here: https://cs.chromium.org/chromium/src/chromecast/media/cma/backend/multizone_b... I'd suggest setting shard_timeout in gn for this target?
The CQ bit was checked by alokp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I have added the shard_timeout. Please review (and test if possible)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
24 hour friendly ping :)
On 2017/08/01 22:01:46, alokp wrote: > 24 hour friendly ping :) Not sure what all happened since I last tested it, but to make this work on Android you'll need cast_media_unittests to depend on "//chromecast/media/cma/backend/android:audio_track_java". Other than that, lgtm
On 2017/08/01 22:50:32, thoren wrote: > On 2017/08/01 22:01:46, alokp wrote: > > 24 hour friendly ping :) > > Not sure what all happened since I last tested it, but to make this work on > Android you'll need cast_media_unittests to depend on > "//chromecast/media/cma/backend/android:audio_track_java". Other than that, lgtm How come it is passing cast_shell_android trybot? Would you mind taking over this patch? It is hard to iterate on it without a way to test locally or a trybot?
On 2017/08/01 23:16:37, alokp wrote: > On 2017/08/01 22:50:32, thoren wrote: > > On 2017/08/01 22:01:46, alokp wrote: > > > 24 hour friendly ping :) > > > > Not sure what all happened since I last tested it, but to make this work on > > Android you'll need cast_media_unittests to depend on > > "//chromecast/media/cma/backend/android:audio_track_java". Other than that, > lgtm > > How come it is passing cast_shell_android trybot? > > Would you mind taking over this patch? It is hard to iterate on it without a way > to test locally or a trybot? The dependency isn't needed until runtime, when some JNI code goes looking for classes defined by audio_track_java. Sure thing, I made a new patch because I wasn't sure how to take this one, and we've switched to Gerrit since this one was made anyway. https://chromium-review.googlesource.com/c/598455 |
