[Cronet] Fix ExperimentalOptionsTest flake

[Cronet] Fix ExperimentalOptionsTest flake

File writes happen on a file thread. This CL waits for the file thread to flush
writes to disk.

BUG=712307
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2827673002
Cr-Commit-Position: refs/heads/master@{#465725}
Committed: https://chromium.googlesource.com/chromium/src/+/5270fa09fc42b95af338780c1406fb2c56eb56ef

[xunjieli, 2017-04-18 15:53:40]

Description was changed from

==========
[Cronet] Fix ExperimentalOptionsTest flake

File writes happen on a file thread. This CL waits for the file thread to flush
writes to disk.

BUG=712307
==========

to

==========
[Cronet] Fix ExperimentalOptionsTest flake

File writes happen on a file thread. This CL waits for the file thread to flush
writes to disk.

BUG=712307
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-04-18 15:54:03]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[xunjieli, 2017-04-18 15:54:04]

Andrei: PTAL. Thanks.

[kapishnikov, 2017-04-18 16:24:34]

Helen, thanks for fixing it. One suggestion below.

https://codereview.chromium.org/2827673002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
(right):

https://codereview.chromium.org/2827673002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:73:
Thread.sleep(10000);
This will increase the duration of every test run by 10 seconds. I would
suggest:
1) Check first and sleep only if the file doesn't contain the string.
2) Decrease the sleep interval from 10 seconds to 100 ms.
3) Set max waiting interval for 5 seconds and fail the test if the string is
still not in the file.

[xunjieli, 2017-04-18 18:30:29]

Thanks, PTAL.

https://codereview.chromium.org/2827673002/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
(right):

https://codereview.chromium.org/2827673002/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:73:
Thread.sleep(10000);
On 2017/04/18 16:24:34, kapishnikov wrote:
> This will increase the duration of every test run by 10 seconds. I would
> suggest:
> 1) Check first and sleep only if the file doesn't contain the string.
> 2) Decrease the sleep interval from 10 seconds to 100 ms.
> 3) Set max waiting interval for 5 seconds and fail the test if the string is
> still not in the file.

Done. Great idea! Per offline discussion, I have changed the while(true) to try
5 times with 100ms interval.

[kapishnikov, 2017-04-18 18:50:01]

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
(right):

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:72:
for (int i = 0; i < 5; i++) {
This loop is a good candidate to be refactored into a separate method like:
assertFileContainsString(logfile, string).

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:80:
assertTrue("File content doesn't match", contains);
Can we add the file name and the searched string to the error message? I think
it is okay not to delete the file from the device in case of a failure, so we
can open it to investigate the cause. Alternatively, we could dump the content
of the file if it is not too big.

[xunjieli, 2017-04-19 19:02:52]

Thanks, PTAL.

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java
(right):

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:72:
for (int i = 0; i < 5; i++) {
On 2017/04/18 18:50:00, kapishnikov wrote:
> This loop is a good candidate to be refactored into a separate method like:
> assertFileContainsString(logfile, string).

Done. Good idea!

https://codereview.chromium.org/2827673002/diff/40001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/ExperimentalOptionsTest.java:80:
assertTrue("File content doesn't match", contains);
On 2017/04/18 18:50:00, kapishnikov wrote:
> Can we add the file name and the searched string to the error message? I think
> it is okay not to delete the file from the device in case of a failure, so we
> can open it to investigate the cause. Alternatively, we could dump the content
> of the file if it is not too big.

Done.

[kapishnikov, 2017-04-19 19:19:56]

lgtm

[xunjieli, 2017-04-19 19:33:26]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-04-19 19:34:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 20:11:13]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492630406657520,
"parent_rev": "469d00d60d18601622c8202af224d25b922ebf48", "commit_rev":
"5270fa09fc42b95af338780c1406fb2c56eb56ef"}

[commit-bot: I haz the power, 2017-04-19 20:11:29]

Description was changed from

==========
[Cronet] Fix ExperimentalOptionsTest flake

File writes happen on a file thread. This CL waits for the file thread to flush
writes to disk.

BUG=712307
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Fix ExperimentalOptionsTest flake

File writes happen on a file thread. This CL waits for the file thread to flush
writes to disk.

BUG=712307
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2827673002
Cr-Commit-Position: refs/heads/master@{#465725}
Committed:
https://chromium.googlesource.com/chromium/src/+/5270fa09fc42b95af338780c1406...
==========

[commit-bot: I haz the power, 2017-04-19 20:11:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5270fa09fc42b95af338780c1406...