[Chromecast] Use ScopedTemp[File|Dir] in tests.

[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597
Bug: b/22953577

Committed: https://crrev.com/ef83eab02623a35a59fe181addb2fae1b0436efc
Cr-Commit-Position: refs/heads/master@{#363875}

[slan, 2015-11-30 16:41:22]

slan@chromium.org changed reviewers:
+ bcf@chromium.org, halliwell@chromium.org

[slan, 2015-11-30 16:41:23]

This change ensures that files and directories created for testing are deleted
when the scope of the test exits.

PTAL

[slan, 2015-11-30 16:43:24]

slan@chromium.org changed reviewers:
+ maruel@chromium.org

[slan, 2015-11-30 16:43:24]

+maruel@ for visibility on crbug.com/561597

[slan, 2015-11-30 17:39:32]

slan@chromium.org changed reviewers:
+ lcwu@chromium.org

[slan, 2015-11-30 17:39:32]

+lcwu@ for OWNERS while halliwell@ is OOO

[M-A Ruel, 2015-11-30 18:27:32]

On 2015/11/30 17:39:32, slan wrote:
> +lcwu@ for OWNERS while halliwell@ is OOO

Thanks for looking at this! (I haven't reviewed)

What is chromecast/base ?

[slan, 2015-11-30 18:34:07]

On 2015/11/30 18:27:32, M-A Ruel wrote:
> On 2015/11/30 17:39:32, slan wrote:
> > +lcwu@ for OWNERS while halliwell@ is OOO
> 
> Thanks for looking at this! (I haven't reviewed)
> 
> What is chromecast/base ?

It is a directory which contains certain utilities useful to Chromecast builds,
but which aren't necessarily useful outside the chromecast/ directory.

[M-A Ruel, 2015-11-30 21:37:44]

On 2015/11/30 18:34:07, slan wrote:
> On 2015/11/30 18:27:32, M-A Ruel wrote:
> > On 2015/11/30 17:39:32, slan wrote:
> > > +lcwu@ for OWNERS while halliwell@ is OOO
> > 
> > Thanks for looking at this! (I haven't reviewed)
> > 
> > What is chromecast/base ?
> 
> It is a directory which contains certain utilities useful to Chromecast
builds,
> but which aren't necessarily useful outside the chromecast/ directory.

I guess my question is more why not use //base/files/scoped_temp_dir.h instead
of copy pasting the code?

[slan, 2015-11-30 21:44:18]

On 2015/11/30 21:37:44, M-A Ruel wrote:
> On 2015/11/30 18:34:07, slan wrote:
> > On 2015/11/30 18:27:32, M-A Ruel wrote:
> > > On 2015/11/30 17:39:32, slan wrote:
> > > > +lcwu@ for OWNERS while halliwell@ is OOO
> > > 
> > > Thanks for looking at this! (I haven't reviewed)
> > > 
> > > What is chromecast/base ?
> > 
> > It is a directory which contains certain utilities useful to Chromecast
> builds,
> > but which aren't necessarily useful outside the chromecast/ directory.
> 
> I guess my question is more why not use //base/files/scoped_temp_dir.h instead
> of copy pasting the code?

Please note that this CL introduces the use of two classes: ScopedTempDir and
ScopedTempFile. The former is declared in //base and is being used per
crbug.com/561597. The latter is merely a convenience class, to increase
readability and help clean up after the tests. I could not find a suitable
substitute in //base.

I could move this class into //base, if you think that would be more fitting,
but I would prefer to do that in a separate CL. WDYT?

Thanks for the quick review :)

[M-A Ruel, 2015-11-30 21:54:30]

Description was changed from

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577
==========

to

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577
==========

[M-A Ruel, 2015-11-30 21:54:30]

maruel@chromium.org changed reviewers:
+ brettw@chromium.org

[M-A Ruel, 2015-11-30 21:56:55]

On 2015/11/30 21:44:18, slan wrote:
> On 2015/11/30 21:37:44, M-A Ruel wrote:
> > On 2015/11/30 18:34:07, slan wrote:
> > > On 2015/11/30 18:27:32, M-A Ruel wrote:
> > > > On 2015/11/30 17:39:32, slan wrote:
> > > > > +lcwu@ for OWNERS while halliwell@ is OOO
> > > > 
> > > > Thanks for looking at this! (I haven't reviewed)
> > > > 
> > > > What is chromecast/base ?
> > > 
> > > It is a directory which contains certain utilities useful to Chromecast
> > builds,
> > > but which aren't necessarily useful outside the chromecast/ directory.
> > 
> > I guess my question is more why not use //base/files/scoped_temp_dir.h
instead
> > of copy pasting the code?
> 
> Please note that this CL introduces the use of two classes: ScopedTempDir and
> ScopedTempFile. The former is declared in //base and is being used per
> crbug.com/561597. The latter is merely a convenience class, to increase
> readability and help clean up after the tests. I could not find a suitable
> substitute in //base.
> 
> I could move this class into //base, if you think that would be more fitting,
> but I would prefer to do that in a separate CL. WDYT?
> 
> Thanks for the quick review :)

I don't think creating the file in ScopedTempFile constructor is a good idea. In
particular there are ScopedTempFile in chromecast/base/serializers_unittest.cc
and chrome/browser/printing/pdf_to_emf_converter.cc so if you want to go this
route, it'd be good to refactor these too.

+Brett for guidance about this.

[slan, 2015-11-30 22:10:48]

> I don't think creating the file in ScopedTempFile constructor is a good idea. 

I am open to refactoring this. What do you suggest?

> In particular there are ScopedTempFile in
chromecast/base/serializers_unittest.cc
> and chrome/browser/printing/pdf_to_emf_converter.cc so if you want to go this
> route, it'd be good to refactor these too.

This CL moves ScopedTempFile from serializers_unittest.cc to scoped_temp_file.*,
for use among other chromecast/ tests. As for the other use case, it has the
same name but serves a different purpose:

typedef scoped_ptr<base::File, BrowserThread::DeleteOnFileThread>
ScopedTempFile;

I don't think that we need a common class here.

[slan, 2015-12-04 23:09:52]

On 2015/11/30 22:10:48, slan wrote:
> > I don't think creating the file in ScopedTempFile constructor is a good
idea. 
> 
> I am open to refactoring this. What do you suggest?
> 
> > In particular there are ScopedTempFile in
> chromecast/base/serializers_unittest.cc
> > and chrome/browser/printing/pdf_to_emf_converter.cc so if you want to go
this
> > route, it'd be good to refactor these too.
> 
> This CL moves ScopedTempFile from serializers_unittest.cc to
scoped_temp_file.*,
> for use among other chromecast/ tests. As for the other use case, it has the
> same name but serves a different purpose:
> 
> typedef scoped_ptr<base::File, BrowserThread::DeleteOnFileThread>
> ScopedTempFile;
> 
> I don't think that we need a common class here.

Ping!

[M-A Ruel, 2015-12-04 23:31:43]

On 2015/12/04 23:09:52, slan wrote:
> Ping!

In particular Brett and Le-Chun. I'm not an owner there.

[slan, 2015-12-05 02:22:05]

On 2015/12/04 23:31:43, M-A Ruel wrote:
> On 2015/12/04 23:09:52, slan wrote:
> > Ping!
> 
> In particular Brett and Le-Chun. I'm not an owner there.

brettw@, I am going to remove you from the reviewers list, as this is a pretty
low-level clean-up. Please add yourself back on if you would like to review.

[slan, 2015-12-05 02:22:23]

slan@chromium.org changed reviewers:
- brettw@chromium.org

[slan, 2015-12-05 02:25:09]

On 2015/12/05 02:22:05, slan wrote:
> On 2015/12/04 23:31:43, M-A Ruel wrote:
> > On 2015/12/04 23:09:52, slan wrote:
> > > Ping!
> > 
> > In particular Brett and Le-Chun. I'm not an owner there.
> 
> brettw@, I am going to remove you from the reviewers list, as this is a pretty
> low-level clean-up. Please add yourself back on if you would like to review.

maruel@, I would welcome your perspective on the replacement of
CreateNewTempDirectory w.r.t. crbug.com/561597.

Thanks in advance for your feedback.

... and ping to Cast guys too :)

[halliwell, 2015-12-07 18:44:16]

On 2015/12/05 02:25:09, slan wrote:
> On 2015/12/05 02:22:05, slan wrote:
> > On 2015/12/04 23:31:43, M-A Ruel wrote:
> > > On 2015/12/04 23:09:52, slan wrote:
> > > > Ping!
> > > 
> > > In particular Brett and Le-Chun. I'm not an owner there.
> > 
> > brettw@, I am going to remove you from the reviewers list, as this is a
pretty
> > low-level clean-up. Please add yourself back on if you would like to review.
> 
> maruel@, I would welcome your perspective on the replacement of
> CreateNewTempDirectory w.r.t. crbug.com/561597.
> 
> Thanks in advance for your feedback.
> 
> ... and ping to Cast guys too :)

This lgtm, but I don't see a resolution on this question:

" I don't think creating the file in ScopedTempFile constructor is a good idea.
"
"  > I am open to refactoring this. What do you suggest?"

[slan, 2015-12-08 16:45:51]

> This lgtm, but I don't see a resolution on this question:
> 
> " I don't think creating the file in ScopedTempFile constructor is a good
idea.
> "
> "  > I am open to refactoring this. What do you suggest?"

Ping to maruel@ for follow-up here.

Ping to bcf@ for eyes on crash/ code in particular.

[slan, 2015-12-08 16:45:59]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-08 16:46:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1484713003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1484713003/40001

[commit-bot: I haz the power, 2015-12-08 17:12:16]

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

[commit-bot: I haz the power, 2015-12-08 17:12:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bcf, 2015-12-08 18:56:44]

On 2015/12/08 16:45:51, slan wrote:
> > This lgtm, but I don't see a resolution on this question:
> > 
> > " I don't think creating the file in ScopedTempFile constructor is a good
> idea.
> > "
> > "  > I am open to refactoring this. What do you suggest?"
> 
> Ping to maruel@ for follow-up here.
> 
> Ping to bcf@ for eyes on crash/ code in particular.

lgtm % nit

[bcf, 2015-12-08 18:56:55]

https://codereview.chromium.org/1484713003/diff/40001/chromecast/base/scoped_...
File chromecast/base/scoped_temp_file.cc (right):

https://codereview.chromium.org/1484713003/diff/40001/chromecast/base/scoped_...
chromecast/base/scoped_temp_file.cc:34: ReadFileToString(path_, &result);
How about a CHECK for this?

[slan, 2015-12-08 19:08:48]

https://codereview.chromium.org/1484713003/diff/40001/chromecast/base/scoped_...
File chromecast/base/scoped_temp_file.cc (right):

https://codereview.chromium.org/1484713003/diff/40001/chromecast/base/scoped_...
chromecast/base/scoped_temp_file.cc:34: ReadFileToString(path_, &result);
On 2015/12/08 18:56:55, bcf wrote:
> How about a CHECK for this?

Done.

[M-A Ruel, 2015-12-08 19:12:19]

lgtm

You could have had a method to create the directory; throwing from a constructor
is generally not recommended since it messes everything up but I don't really
mind personally.

https://codereview.chromium.org/1484713003/diff/60001/chromecast/base/scoped_...
File chromecast/base/scoped_temp_file.cc (right):

https://codereview.chromium.org/1484713003/diff/60001/chromecast/base/scoped_...
chromecast/base/scoped_temp_file.cc:32: CHECK(FileExists());
You don't need this line anymore IIUC.

[slan, 2015-12-08 19:18:07]

On 2015/12/08 19:12:19, M-A Ruel wrote:
> You could have had a method to create the directory; throwing from a
constructor
> is generally not recommended since it messes everything up but I don't really
> mind personally.

Agreed that doing stuff like this from the ctor is not generally good style.
However, since this is a test and we are bluntly crashing the process anyway,
I'll leave it here for simplicity.

[slan, 2015-12-08 19:18:24]

https://codereview.chromium.org/1484713003/diff/60001/chromecast/base/scoped_...
File chromecast/base/scoped_temp_file.cc (right):

https://codereview.chromium.org/1484713003/diff/60001/chromecast/base/scoped_...
chromecast/base/scoped_temp_file.cc:32: CHECK(FileExists());
On 2015/12/08 19:12:19, M-A Ruel wrote:
> You don't need this line anymore IIUC.

That's a good point, but I think I will keep this redundant check here for
clarity to the developer, and to maintain consistency with Write(). That way, if
line 34 fails, we know that it's not because the file has been moved out from
under us - it's due to some other IO error.

[slan, 2015-12-08 19:18:31]

The CQ bit was checked by slan@chromium.org

[slan, 2015-12-08 19:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, bcf@chromium.org
Link to the patchset: https://codereview.chromium.org/1484713003/#ps60001
(title: "Fix bug in test fixture, add CHECK to ScopedTempFile::Read")

[commit-bot: I haz the power, 2015-12-08 19:20:08]

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

[commit-bot: I haz the power, 2015-12-08 19:49:40]

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

[commit-bot: I haz the power, 2015-12-08 19:49:41]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[slan, 2015-12-08 21:31:34]

On 2015/12/08 19:49:41, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Trybot failures appear to be unrelated, will wait until the tree greens up a bit
to submit.

[slan, 2015-12-08 22:53:04]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-12-08 22:54:52]

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

[commit-bot: I haz the power, 2015-12-09 00:59:38]

Description was changed from

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577
==========

to

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577
==========

[commit-bot: I haz the power, 2015-12-09 00:59:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-09 01:00:58]

Description was changed from

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577
==========

to

==========
[Chromecast] Use ScopedTemp[File|Dir] in tests.

Files and directories created in a test should be removed as soon as the
test is done executing. Replace all uses of
base::CreateNewTempDirectory() with base::ScopedTempDir. Also introduce
a ScopedTempFile to ensure automatic clean-up of files created for
testing.

BUG=561597

Bug: b/22953577

Committed: https://crrev.com/ef83eab02623a35a59fe181addb2fae1b0436efc
Cr-Commit-Position: refs/heads/master@{#363875}
==========

[commit-bot: I haz the power, 2015-12-09 01:00:59]

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