Reduce dependencies on rtc::FileSystem in FileRotatingStream tests, adding helpers in webrtc::test::

Reduce dependencies on rtc::FileSystem in FileRotatingStream tests.

Use webrtc::test::OutputPath instead of Filesystem::GetAppTempFolder.
Added functions RemoveFile and RemoveDir in the webrtc::test namespace,
to replace use of Filesystem::DeleteFolderAndContents.

This makes Filesystem::DeleteFolderAndContents unused, to be deleted
together with related code in a followup cl.

BUG=webrtc:7345
Review-Url: https://codereview.webrtc.org/2872283002
Cr-Commit-Position: refs/heads/master@{#18173}
Committed: https://chromium.googlesource.com/external/webrtc/+/dd7b5f32b59d9def668b9e9487589a572f20f6e0

[nisse-webrtc, 2017-05-10 12:02:37]

nisse@webrtc.org changed reviewers:
+ kjellander@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2017-05-10 12:02:38]

PTAL. If we can land this, quite a lot of code in base/fileutils.cc becomes
unused.

What about the test log files? Is it important that the test itself deletes
them?

[kjellander_webrtc, 2017-05-10 13:55:52]

On 2017/05/10 12:02:38, nisse-webrtc wrote:
> PTAL. If we can land this, quite a lot of code in base/fileutils.cc becomes
> unused.
> 
> What about the test log files? Is it important that the test itself deletes
> them?

It's not the end of the world if it doesn't, but it's a policy we've been trying
to push. To be sure (since many tests used to leave a round a lot of files), the
bots have a cleanup dir step happening for each build though, to avoid filling
up the disk.
Please do it if it's possible.

[nisse-webrtc, 2017-05-11 08:04:24]

On 2017/05/10 13:55:52, kjellander_webrtc wrote:
> On 2017/05/10 12:02:38, nisse-webrtc wrote:
> > PTAL. If we can land this, quite a lot of code in base/fileutils.cc becomes
> > unused.
> > 
> > What about the test log files? Is it important that the test itself deletes
> > them?
> 
> It's not the end of the world if it doesn't, but it's a policy we've been
trying
> to push. To be sure (since many tests used to leave a round a lot of files),
the
> bots have a cleanup dir step happening for each build though, to avoid filling
> up the disk.
> Please do it if it's possible.

Done. Waiting for the bots to try it out on windows.

The w32 implementation of RemoveFile and RemoveDirectory is my best guess.

[nisse-webrtc, 2017-05-11 09:18:25]

nisse@webrtc.org changed reviewers:
+ tommi@webrtc.org

[nisse-webrtc, 2017-05-11 09:18:25]

Tommi, can you have a look? I have little idea on how the posixish _unlink and
_rmdir functions really work on windows.

[nisse-webrtc, 2017-05-12 08:51:25]

Ping?

[kjellander_webrtc, 2017-05-12 09:02:24]

lgtm but consider updating the title+description since we're now adding features
to fileutils.h

[nisse-webrtc, 2017-05-12 09:08:50]

Description was changed from

==========
Reduce dependencies on rtc::FileSystem in FileRotatingStream tests.

Use webrtc::test::OutputPath instead of Filesystem::GetAppTempFolder.
Don't use Filesystem::DeleteFolderAndContents.

BUG=webrtc:7345
==========

to

==========
Reduce dependencies on rtc::FileSystem in FileRotatingStream tests.

Use webrtc::test::OutputPath instead of Filesystem::GetAppTempFolder.
Added functions RemoveFile and RemoveDir in the webrtc::test namespace,
to replace use of Filesystem::DeleteFolderAndContents.

This makes Filesystem::DeleteFolderAndContents unused, to be deleted
together with related code in a followup cl.

BUG=webrtc:7345
==========

[tommi, 2017-05-12 10:47:16]

Could DeleteFolderAndContents simply be moved into the place it's currently
being used from?

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:237: return _rmdir(directory_name.c_str())
== 0;
Use RemoveDirectoryA()
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365488(v=vs.85).aspx

Btw, is this supposed to delete only empty directories or might the directory
contain files?  RemoveDirectory only removes the directory but if it has files,
it cannot be removed. Files need to be recursively deleted first.

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:245: return _unlink(file_name.c_str()) ==
0;
DeleteFileA
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363915(v=vs.85).aspx

[nisse-webrtc, 2017-05-12 11:06:15]

On 2017/05/12 10:47:16, tommi (wëbrtc) wrote:
> Could DeleteFolderAndContents simply be moved into the place it's currently
> being used from?

Possible, but a bit pointless in my opinion. There's nothing that needs it
recursive deletion of files and directories.

> Btw, is this supposed to delete only empty directories or might the directory
> contain files? 

The new function is intended to only delete empty directories. The cleanup code
gets the list of files and deletes 
them all first, then the directory is deleted. If there are files left at that
point, the test should fail.

[nisse-webrtc, 2017-05-12 11:19:00]

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:237: return _rmdir(directory_name.c_str())
== 0;
On 2017/05/12 10:47:15, tommi (wëbrtc) wrote:
> Use RemoveDirectoryA()
>
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365488(v=vs.85).aspx

Done.

https://codereview.webrtc.org/2872283002/diff/60001/webrtc/test/testsupport/f...
webrtc/test/testsupport/fileutils.cc:245: return _unlink(file_name.c_str()) ==
0;
On 2017/05/12 10:47:15, tommi (wëbrtc) wrote:
> DeleteFileA
>
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363915(v=vs.85).aspx

Done.

[nisse-webrtc, 2017-05-12 12:00:05]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 12:00:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-12 12:17:26]

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

[commit-bot: I haz the power, 2017-05-12 12:17:28]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-05-16 13:06:07]

Ping? I think this is ready, by I'd need OWNER's approval by Peter or Tommi.

[tommi, 2017-05-16 14:34:22]

lgtm

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
File webrtc/base/filerotatingstream_unittest.cc (right):

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
webrtc/base/filerotatingstream_unittest.cc:27: stream->Close();
nit: it feels kinda strange to do this inside here and not before the function
is called, but no biggie.

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
webrtc/base/filerotatingstream_unittest.cc:28: for (size_t i = 0; i <
stream->GetNumFiles(); i++) {
++i

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/test/testsupport/...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/test/testsupport/...
webrtc/test/testsupport/fileutils.cc:237: return
RemoveDirectoryA(directory_name.c_str()) != 0;
nit: != FALSE;

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/test/testsupport/...
webrtc/test/testsupport/fileutils.cc:245: return DeleteFileA(file_name.c_str())
!= 0;
ditto

[nisse-webrtc, 2017-05-17 07:38:32]

Addressed nits, attempting to land now.

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
File webrtc/base/filerotatingstream_unittest.cc (right):

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
webrtc/base/filerotatingstream_unittest.cc:27: stream->Close();
On 2017/05/16 14:34:21, tommi (wëbrtc) wrote:
> nit: it feels kinda strange to do this inside here and not before the function
> is called, but no biggie.

Done.

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/base/filerotating...
webrtc/base/filerotatingstream_unittest.cc:28: for (size_t i = 0; i <
stream->GetNumFiles(); i++) {
On 2017/05/16 14:34:21, tommi (wëbrtc) wrote:
> ++i

Done.

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/test/testsupport/...
File webrtc/test/testsupport/fileutils.cc (right):

https://codereview.webrtc.org/2872283002/diff/100001/webrtc/test/testsupport/...
webrtc/test/testsupport/fileutils.cc:237: return
RemoveDirectoryA(directory_name.c_str()) != 0;
On 2017/05/16 14:34:21, tommi (wëbrtc) wrote:
> nit: != FALSE;

Done.

[nisse-webrtc, 2017-05-17 07:38:39]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-17 07:38:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2872283002/#ps120001 (title:
"Addressed nits.")

[commit-bot: I haz the power, 2017-05-17 07:38:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-17 08:08:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495006719368790,
"parent_rev": "98e186c71c10fa7d221fa82391865be1adfdff72", "commit_rev":
"dd7b5f32b59d9def668b9e9487589a572f20f6e0"}

[commit-bot: I haz the power, 2017-05-17 08:08:41]

Description was changed from

==========
Reduce dependencies on rtc::FileSystem in FileRotatingStream tests.

Use webrtc::test::OutputPath instead of Filesystem::GetAppTempFolder.
Added functions RemoveFile and RemoveDir in the webrtc::test namespace,
to replace use of Filesystem::DeleteFolderAndContents.

This makes Filesystem::DeleteFolderAndContents unused, to be deleted
together with related code in a followup cl.

BUG=webrtc:7345
==========

to

==========
Reduce dependencies on rtc::FileSystem in FileRotatingStream tests.

Use webrtc::test::OutputPath instead of Filesystem::GetAppTempFolder.
Added functions RemoveFile and RemoveDir in the webrtc::test namespace,
to replace use of Filesystem::DeleteFolderAndContents.

This makes Filesystem::DeleteFolderAndContents unused, to be deleted
together with related code in a followup cl.

BUG=webrtc:7345

Review-Url: https://codereview.webrtc.org/2872283002
Cr-Commit-Position: refs/heads/master@{#18173}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dd7b5f32b59d9def668b9e948...
==========

[commit-bot: I haz the power, 2017-05-17 08:08:42]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/dd7b5f32b59d9def668b9e948...

[charujain1, 2017-05-17 11:52:05]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/2892453003/ by charujain@google.com.

The reason for reverting is: Fails to compile successfully. .

[ehmaldonado_webrtc, 2017-05-17 12:22:01]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.webrtc.org/2885393002/ by ehmaldonado@webrtc.org.

The reason for reverting is: Fails to compile successfully. 
.