Start from clean state in DownloadTests

Start from clean state in DownloadTests

Clear all files that could conflict with the tests execution
from download directory.

BUG=618067
Committed: https://crrev.com/6683d6962f36ad95829b54f3e5f4be18aa2b12f7
Cr-Commit-Position: refs/heads/master@{#398943}

[kraush, 2016-06-07 20:44:59]

kraush@amazon.com changed reviewers:
+ dtrainor@chromium.org, qinmin@chromium.org

[kraush, 2016-06-07 20:45:00]

Hi David, Hi qinmin,

Can you please take a look at this test setup change? :)

Thanks!
Holger

[qinmin, 2016-06-08 17:12:58]

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

https://codereview.chromium.org/2043743004/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:440:
deleteFilesInDownloadDirectory(FILENAME_WALLPAPER, FILENAME_TEXT,
FILENAME_TEXT_1,
It would be better move these constants into an array, so that you can do a loop
to delete them

[kraush, 2016-06-08 17:17:00]

On 2016/06/08 17:12:58, qinmin wrote:
>
https://codereview.chromium.org/2043743004/diff/1/chrome/android/javatests/sr...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
> (right):
> 
>
https://codereview.chromium.org/2043743004/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:440:
> deleteFilesInDownloadDirectory(FILENAME_WALLPAPER, FILENAME_TEXT,
> FILENAME_TEXT_1,
> It would be better move these constants into an array, so that you can do a
loop
> to delete them

Sure, sounds good!
Which of those approaches would you prefer?
a) Keep them as separate constants and assign all the constants to an array
(e.g. TEST_FILES), then call deleteFilesInDownloadDirectory with the array
rather than th list of strings
b) Remove the constants and only create the array. Pass the array to
deleteFilesInDownloadDirectory and even in the tests directly use e.g.
TEST_FILES[0]

[qinmin, 2016-06-08 17:45:53]

On 2016/06/08 17:17:00, kraush wrote:
> On 2016/06/08 17:12:58, qinmin wrote:
> >
>
https://codereview.chromium.org/2043743004/diff/1/chrome/android/javatests/sr...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2043743004/diff/1/chrome/android/javatests/sr...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:440:
> > deleteFilesInDownloadDirectory(FILENAME_WALLPAPER, FILENAME_TEXT,
> > FILENAME_TEXT_1,
> > It would be better move these constants into an array, so that you can do a
> loop
> > to delete them
> 
> Sure, sounds good!
> Which of those approaches would you prefer?
> a) Keep them as separate constants and assign all the constants to an array
> (e.g. TEST_FILES), then call deleteFilesInDownloadDirectory with the array
> rather than th list of strings
> b) Remove the constants and only create the array. Pass the array to
> deleteFilesInDownloadDirectory and even in the tests directly use e.g.
> TEST_FILES[0]

I would prefer a) since these names can be used somewhere in the test directly.
b) would require devs to add files to the end of the array if there are more
tests to be added.

[kraush, 2016-06-08 21:22:33]

Hi qinmin,

I've added the constants to arrays as you suggested.
This should make it easier for others to update the file list in case they add
new files.

- Holger

[David Trainor- moved to gerrit, 2016-06-09 02:55:42]

lgtm, but wait for qinmin@ to reply.  I'm tempted to ask for this in tearDown
too, so when all the tests finish they have no impact on the downloads folder. 
But up to you.

[kraush, 2016-06-09 14:57:19]

On 2016/06/09 02:55:42, David Trainor wrote:
> lgtm, but wait for qinmin@ to reply.  I'm tempted to ask for this in tearDown
> too, so when all the tests finish they have no impact on the downloads folder.

> But up to you.

Thanks, good idea David! :)
I'll refactor it out into a method and call that method in both setUp and
tearDown.

[kraush, 2016-06-09 15:20:15]

Added cleanup to both setUp as well as tearDown (dtrainor@)

[qinmin, 2016-06-09 16:48:12]

lgtm % nit

https://codereview.chromium.org/2043743004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java
(right):

https://codereview.chromium.org/2043743004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/download/DownloadTest.java:85:
assertEquals(mTestServer.getURL("/chrome/test/data/android/download/" +
FILENAME_GZIP),
nit: make "/chrome/test/data/android/download/ " a constant

[kraush, 2016-06-09 17:05:43]

Turned /chrome/test/data/android/download/ into a constant (qinmin@)

Submitting this revision.

[kraush, 2016-06-09 17:05:51]

The CQ bit was checked by kraush@amazon.com

[kraush, 2016-06-09 17:05:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2043743004/#ps60001
(title: "Turned /chrome/test/data/android/download/ into a constant")

[commit-bot: I haz the power, 2016-06-09 17:06:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043743004/60001

[commit-bot: I haz the power, 2016-06-09 18:07:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-09 18:07:44]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 18:10:14]

Description was changed from

==========
Start from clean state in DownloadTests

Clear all files that could conflict with the tests execution
from download directory.

BUG=618067
==========

to

==========
Start from clean state in DownloadTests

Clear all files that could conflict with the tests execution
from download directory.

BUG=618067

Committed: https://crrev.com/6683d6962f36ad95829b54f3e5f4be18aa2b12f7
Cr-Commit-Position: refs/heads/master@{#398943}
==========

[commit-bot: I haz the power, 2016-06-09 18:10:15]

Patchset 4 (id:??) landed as
https://crrev.com/6683d6962f36ad95829b54f3e5f4be18aa2b12f7
Cr-Commit-Position: refs/heads/master@{#398943}