Reenable the WebRTC EventLog API Tests.

Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is produced, or the filename is different than expected.

BUG=webrtc:4741
Committed: https://crrev.com/f79d7975850ec7859230fa286601efc8b84042ec
Cr-Commit-Position: refs/heads/master@{#407119}

[Ivo-OOO until feb 6, 2016-07-08 09:31:13]

Description was changed from

==========
Reenable and improve WebRTC EventLog API Test.

BUG=webrtc:4741
==========

to

==========
Reenable and improve WebRTC EventLog API Test.

BUG=webrtc:4741
==========

[Ivo-OOO until feb 6, 2016-07-08 09:31:13]

ivoc@chromium.org changed reviewers:
+ grunell@chromium.org, terelius@chromium.org

[Ivo-OOO until feb 6, 2016-07-08 09:36:39]

This CL reenables the WebRTC API tests. I added a timeout to the infinite
while-loop, so the test won't get stuck if the event log functionality becomes
broken or the filename is different from the expected name.
I also had to update the expected filename to match the new behavior. 
Bjorn: do you know if this new naming scheme should be updated anywhere else in
Chrome, or if it will cause any problems?

[Ivo-OOO until feb 6, 2016-07-08 09:36:56]

Description was changed from

==========
Reenable and improve WebRTC EventLog API Test.

BUG=webrtc:4741
==========

to

==========
Reenable the WebRTC EventLog API Tests.

BUG=webrtc:4741
==========

[Ivo-OOO until feb 6, 2016-07-08 10:53:38]

Description was changed from

==========
Reenable the WebRTC EventLog API Tests.

BUG=webrtc:4741
==========

to

==========
Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is
produced, or the filename is different than expected.

BUG=webrtc:4741
==========

[terelius1, 2016-07-08 10:58:36]

terelius@google.com changed reviewers:
+ terelius@google.com

[terelius1, 2016-07-08 10:58:37]

Changing the filename should be fine as long as it matches what is produced by
WebrtcEventLogHandler.

Btw, does the webrtc-internals page and the extension API use the same naming
convention?

https://codereview.chromium.org/2131133002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:41:
// <temporary path>.<render process id>.event_log.<consumer id>, for example
Please update the comment.

[Ivo-OOO until feb 6, 2016-07-08 11:24:35]

> Changing the filename should be fine as long as it matches what is produced by
WebrtcEventLogHandler.

I was worried that there might be other places in Chrome with a function that
tries to predict the name of the generated file (maybe to upload it?). Also, the
filename is modified both by WebRtcEventLogHandler and WebRTCEventLogHost now.

> Btw, does the webrtc-internals page and the extension API use the same naming
convention?

Not sure what naming convention you mean?

https://codereview.chromium.org/2131133002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:41:
// <temporary path>.<render process id>.event_log.<consumer id>, for example
On 2016/07/08 10:58:37, terelius1 wrote:
> Please update the comment.

Done. I renamed the variable below as well.

[Ivo-OOO until feb 6, 2016-07-19 09:04:08]

ivoc@chromium.org changed reviewers:
+ asargent@chromium.org
- grunell@chromium.org

[Ivo-OOO until feb 6, 2016-07-19 09:08:34]

Adding asargent@ as reviewer instead of grunell@, because grunell@ is currently
on vacation. PTAL.

[asargent_no_longer_on_chrome, 2016-07-19 16:29:54]

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:192:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
It's usually problematic to use sleep calls inside tests. Can you switch to
using base::FilePathWatcher instead here?

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:271:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
same thing here

[Ivo-OOO until feb 6, 2016-07-20 12:12:33]

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:192:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
On 2016/07/19 16:29:54, Antony Sargent wrote:
> It's usually problematic to use sleep calls inside tests. Can you switch to
> using base::FilePathWatcher instead here?

Good idea, it looks like FilePathWatcher would be a good fit here, but I'm
worried that the test can get stuck here if the file for some reason doesn't get
created (which actually happened recently). Do you know how that case would be
handled with FilePathWatcher? Or is it okay if tests can get stuck?

[asargent_no_longer_on_chrome, 2016-07-20 17:19:12]

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:192:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
On 2016/07/20 12:12:33, Ivo wrote:
> On 2016/07/19 16:29:54, Antony Sargent wrote:
> > It's usually problematic to use sleep calls inside tests. Can you switch to
> > using base::FilePathWatcher instead here?
> 
> Good idea, it looks like FilePathWatcher would be a good fit here, but I'm
> worried that the test can get stuck here if the file for some reason doesn't
get
> created (which actually happened recently). Do you know how that case would be
> handled with FilePathWatcher? Or is it okay if tests can get stuck?


I think you can just use a base::RunLoop with a helper class that contains the
FilePathWatcher - the RunLoop will automatically get quit if the test times out.
Something like the following:

class MyWatcher {
 public:
  MyWatcher(const base::FilePath& path) : found_(false), path_(path) {}

  
  bool Start() {
    if (base::PathExists(path_)) {
      found_ = true;
      return true;
    } else {
      return watcher_.Watch(path_, false /* recursive */, 
                            base::Bind(this, &Watcher::Callback));
    }
  }

  // Returns true if |path_| became available.
  bool Wait() {
    if (!found_) {
      run_loop_.Run();
    }
    return found_;
  }  

  // implements FilePathWatcher::Callback
  void Callback(const FilePath& path, bool error) {
    EXPECT_EQ(path, path_);
    if (!error)
      found_ = true;
    run_loop_.Quit();
  }
  
 
 private:
  base::RunLoop run_loop_;
  bool found_;
  base::FilePath path_;
  base::FilePathWatcher watcher_;
};


then you can use this in your test like this:

MyWatcher watcher(full_file_name);
ASSERT_TRUE(watcher.Start()) << "error watching for " << full_file_name;

// do stuff that can result in logfile becoming available

ASSERT_TRUE(watcher.WaitForFile());


Then the test will either abort on timeouts, or continue from this point knowing
the file exists.

One problem with all this is that the file might not have the contents you want
yet - I think the MyWatcher Wait method might return as soon as the file gets
created, but there might not be a guarantee that it has any contents yet. To get
around this, the easiest thing to do might be to add some more explicit Watcher
interface from the code which writes the log in the first place, which gets
triggered once it is done writing the log file.

If that isn't feasible, you could get fancier with your use of FilePathWatcher
to wait for contents to be written into the file, but that seems inherently
error prone (at least if you care about how many bytes are written or whether
they have specific contents), and also runs into the problem that on Mac OSX it
sounds like FilePathWatcher only fires for file creation/deletion but not
contents changes.

[Ivo-OOO until feb 6, 2016-07-21 15:20:55]

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:192:
base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
On 2016/07/20 17:19:12, Antony Sargent wrote:
> On 2016/07/20 12:12:33, Ivo wrote:
> > On 2016/07/19 16:29:54, Antony Sargent wrote:
> > > It's usually problematic to use sleep calls inside tests. Can you switch
to
> > > using base::FilePathWatcher instead here?
> > 
> > Good idea, it looks like FilePathWatcher would be a good fit here, but I'm
> > worried that the test can get stuck here if the file for some reason doesn't
> get
> > created (which actually happened recently). Do you know how that case would
be
> > handled with FilePathWatcher? Or is it okay if tests can get stuck?
> 
> 
> I think you can just use a base::RunLoop with a helper class that contains the
> FilePathWatcher - the RunLoop will automatically get quit if the test times
out.
> Something like the following:
> 
> class MyWatcher {
>  public:
>   MyWatcher(const base::FilePath& path) : found_(false), path_(path) {}
> 
>   
>   bool Start() {
>     if (base::PathExists(path_)) {
>       found_ = true;
>       return true;
>     } else {
>       return watcher_.Watch(path_, false /* recursive */, 
>                             base::Bind(this, &Watcher::Callback));
>     }
>   }
> 
>   // Returns true if |path_| became available.
>   bool Wait() {
>     if (!found_) {
>       run_loop_.Run();
>     }
>     return found_;
>   }  
> 
>   // implements FilePathWatcher::Callback
>   void Callback(const FilePath& path, bool error) {
>     EXPECT_EQ(path, path_);
>     if (!error)
>       found_ = true;
>     run_loop_.Quit();
>   }
>   
>  
>  private:
>   base::RunLoop run_loop_;
>   bool found_;
>   base::FilePath path_;
>   base::FilePathWatcher watcher_;
> };
> 
> 
> then you can use this in your test like this:
> 
> MyWatcher watcher(full_file_name);
> ASSERT_TRUE(watcher.Start()) << "error watching for " << full_file_name;
> 
> // do stuff that can result in logfile becoming available
> 
> ASSERT_TRUE(watcher.WaitForFile());
> 
> 
> Then the test will either abort on timeouts, or continue from this point
knowing
> the file exists.
> 
> One problem with all this is that the file might not have the contents you
want
> yet - I think the MyWatcher Wait method might return as soon as the file gets
> created, but there might not be a guarantee that it has any contents yet. To
get
> around this, the easiest thing to do might be to add some more explicit
Watcher
> interface from the code which writes the log in the first place, which gets
> triggered once it is done writing the log file.
> 
> If that isn't feasible, you could get fancier with your use of FilePathWatcher
> to wait for contents to be written into the file, but that seems inherently
> error prone (at least if you care about how many bytes are written or whether
> they have specific contents), and also runs into the problem that on Mac OSX
it
> sounds like FilePathWatcher only fires for file creation/deletion but not
> contents changes. 

Wow, thanks for the detailed reply and code, very helpful! I made the change
like you suggested. 
I don't think we care much about the actual file contents in this test (we have
plenty of other tests inside of WebRTC for that), so just checking that the file
was created and that it contains something should be fine.

[asargent_no_longer_on_chrome, 2016-07-21 23:19:28]

lgtm

https://codereview.chromium.org/2131133002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:62:
class FileWatcher : public base::RefCountedThreadSafe<FileWatcher> {
nit: using the same name as the class in base can be confusing for things like
find/grep, codesearch, etc. even though namespaces let it compile. I'd prefer if
you used some distinct name for this class, perhaps something like
"BlockingFileWatcher" or "FileWaiter" (feel free to use something else if you
have a better idea)

[Ivo-OOO until feb 6, 2016-07-22 09:21:04]

https://codereview.chromium.org/2131133002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc
(right):

https://codereview.chromium.org/2131133002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc:62:
class FileWatcher : public base::RefCountedThreadSafe<FileWatcher> {
On 2016/07/21 23:19:27, Antony Sargent wrote:
> nit: using the same name as the class in base can be confusing for things like
> find/grep, codesearch, etc. even though namespaces let it compile. I'd prefer
if
> you used some distinct name for this class, perhaps something like
> "BlockingFileWatcher" or "FileWaiter" (feel free to use something else if you
> have a better idea) 

Good point, I renamed it to FileWaiter.

[Ivo-OOO until feb 6, 2016-07-22 09:21:19]

The CQ bit was checked by ivoc@chromium.org

[Ivo-OOO until feb 6, 2016-07-22 09:21:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2131133002/#ps60001
(title: "Renamed FileWatcher to FileWaiter")

[commit-bot: I haz the power, 2016-07-22 09:21:36]

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, 2016-07-22 10:01:19]

Description was changed from

==========
Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is
produced, or the filename is different than expected.

BUG=webrtc:4741
==========

to

==========
Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is
produced, or the filename is different than expected.

BUG=webrtc:4741
==========

[commit-bot: I haz the power, 2016-07-22 10:01:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-22 10:03:14]

Description was changed from

==========
Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is
produced, or the filename is different than expected.

BUG=webrtc:4741
==========

to

==========
Reenable the WebRTC EventLog API Tests.

Also adds a timeout to the test, to prevent an infinite loop when no file is
produced, or the filename is different than expected.

BUG=webrtc:4741

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

[commit-bot: I haz the power, 2016-07-22 10:03:15]

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