[Android] Remove test file after download test

[Android] Remove test file after download test

If file left, it could affect other tests that uses the same download
source.

BUG=286315
Committed: https://crrev.com/5f3ecac74cd419bf45a8b0c99e3832281dda6c69
Cr-Commit-Position: refs/heads/master@{#369317}

[boliu, 2016-01-13 22:15:07]

boliu@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[boliu, 2016-01-13 22:15:07]

stamp pls

[gone, 2016-01-13 22:27:12]

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
(right):

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:317:
if (!file.exists()) {
This file really shouldn't ever exist between test runs.  It should be deleted.

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:339:
Log.d(TAG, "Failed to delete test.gzip");
Is this something that should be handled in the setUp() and tearDown() phases? 
Maybe the test should create a temporary test-specific directory so that other
tests can't accidentally make a test.gzip file on its own (e.g. what
TestTabModelDirectory does)?

[boliu, 2016-01-13 22:34:02]

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
(right):

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:317:
if (!file.exists()) {
On 2016/01/13 22:27:12, dfalcantara wrote:
> This file really shouldn't ever exist between test runs.  It should be
deleted.

Well, I assume there are corner cases. Eg native crash in the middle of the test
or something. I can remove the check, but probably also the assertTrue for
createNewFile as well.

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:339:
Log.d(TAG, "Failed to delete test.gzip");
On 2016/01/13 22:27:12, dfalcantara wrote:
> Is this something that should be handled in the setUp() and tearDown() phases?


Not every single test wants this. I can put the remove in tearDown, but then
it's awkward to not match up with a setUp.

> Maybe the test should create a temporary test-specific directory so that other
> tests can't accidentally make a test.gzip file on its own (e.g. what
> TestTabModelDirectory does)?

It has to be the shared download directory, for testing downloading duplicated
files. :/

[gone, 2016-01-13 22:36:32]

lgtm

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
(right):

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:317:
if (!file.exists()) {
On 2016/01/13 22:34:01, boliu wrote:
> On 2016/01/13 22:27:12, dfalcantara wrote:
> > This file really shouldn't ever exist between test runs.  It should be
> deleted.
> 
> Well, I assume there are corner cases. Eg native crash in the middle of the
test
> or something. I can remove the check, but probably also the assertTrue for
> createNewFile as well.

Yeah, I meant that the file should always be explicitly deleted even if it
already exists.  There shouldn't be any state that crosses over between tests
like this, but I don't know what good that'll do in general.

https://codereview.chromium.org/1586593003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:339:
Log.d(TAG, "Failed to delete test.gzip");
On 2016/01/13 22:34:01, boliu wrote:
> On 2016/01/13 22:27:12, dfalcantara wrote:
> > Is this something that should be handled in the setUp() and tearDown()
phases?
> 
> 
> Not every single test wants this. I can put the remove in tearDown, but then
> it's awkward to not match up with a setUp.
> 
> > Maybe the test should create a temporary test-specific directory so that
other
> > tests can't accidentally make a test.gzip file on its own (e.g. what
> > TestTabModelDirectory does)?
> 
> It has to be the shared download directory, for testing downloading duplicated
> files. :/

Eh, not so awkward, but if you're fine with that that's alright.

[boliu, 2016-01-13 22:41:56]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-01-13 22:43:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586593003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586593003/1

[commit-bot: I haz the power, 2016-01-14 00:22:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-14 00:22:25]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)

[boliu, 2016-01-14 02:32:43]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-01-14 02:40:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586593003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586593003/1

[commit-bot: I haz the power, 2016-01-14 03:56:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-14 03:56:59]

Description was changed from

==========
[Android] Remove test file after download test

If file left, it could affect other tests that uses the same download
source.

BUG=286315
==========

to

==========
[Android] Remove test file after download test

If file left, it could affect other tests that uses the same download
source.

BUG=286315

Committed: https://crrev.com/5f3ecac74cd419bf45a8b0c99e3832281dda6c69
Cr-Commit-Position: refs/heads/master@{#369317}
==========

[commit-bot: I haz the power, 2016-01-14 03:56:59]

Patchset 1 (id:??) landed as
https://crrev.com/5f3ecac74cd419bf45a8b0c99e3832281dda6c69
Cr-Commit-Position: refs/heads/master@{#369317}