[Telemetry] Fix video_unittest so that it actually runs on bots.

[Telemetry] Fix video_unittest so that it actually runs on bots.

This also makes it explicitly clear that the avconv dependency is currently linux only.

I had another CL out a while ago that fixes this, but that CL's blocked, so I'm separating this out.

BUG=433943
Committed: https://crrev.com/eae4959498c328f8e3fa710b1d507c0d68225b18
Cr-Commit-Position: refs/heads/master@{#306858}

[mthiesse, 2014-11-26 20:55:58]

mthiesse@chromium.org changed reviewers:
+ slamm@chromium.org, tonyg@chromium.org

[mthiesse, 2014-11-26 20:55:58]

PTAL,

I've separated out re-enabling this test from my bitmap.py change CL.

[slamm, 2014-12-02 21:01:59]

lgtm

[mthiesse, 2014-12-02 21:39:53]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2014-12-02 21:41:13]

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

[commit-bot: I haz the power, 2014-12-02 21:48:34]

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

[commit-bot: I haz the power, 2014-12-02 21:48:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tonyg, 2014-12-02 22:31:02]

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:166:
cloud_storage.GetIfChanged(bin_path, cloud_storage.PUBLIC_BUCKET)
Are all binaries in tools/telemetry/bin/linux/x86_64/* currently in the public
bucket?

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (left):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:22: assert not
video_file_obj.close_called
Can you explain why these are no longer valid?

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video_unittest.py (right):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video_unittest.py:22:
host_platform.InstallApplication('avconv')
Should the unittest really keep calling this? Seems like it should call some
actual Telemetry code that handles it.

[mthiesse, 2014-12-02 23:23:26]

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/linux_platform_backend.py (right):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/linux_platform_backend.py:166:
cloud_storage.GetIfChanged(bin_path, cloud_storage.PUBLIC_BUCKET)
On 2014/12/02 22:31:02, tonyg wrote:
> Are all binaries in tools/telemetry/bin/linux/x86_64/* currently in the public
> bucket?

No, but they should be. I moved avconv to the public bucket so the bots could
access it. Setting the bucket here just tells the cloud storage code to search
the public bucket first, but if the binary is not in the public bucket it will
search all other buckets as well.

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (left):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:22: assert not
video_file_obj.close_called
On 2014/12/02 22:31:02, tonyg wrote:
> Can you explain why these are no longer valid?

I didn't write this code, but these asserts made no sense. video_file_obj,
according to the unit test for this code, is just a string, so it has no delete
or close_called methods.

Perhaps the unit test was just written poorly? Is there some actual video object
that has these methods?

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video_unittest.py (right):

https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video_unittest.py:22:
host_platform.InstallApplication('avconv')
On 2014/12/02 22:31:02, tonyg wrote:
> Should the unittest really keep calling this? Seems like it should call some
> actual Telemetry code that handles it.

I'm not sure I follow you. InstallApplication is the telemetry code that's
supposed to handle the case that an application can't be launched, no?

[tonyg, 2014-12-03 00:54:56]

On 2014/12/02 23:23:26, mthiesse wrote:
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/linux_platform_backend.py
(right):
> 
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/linux_platform_backend.py:166:
> cloud_storage.GetIfChanged(bin_path, cloud_storage.PUBLIC_BUCKET)
> On 2014/12/02 22:31:02, tonyg wrote:
> > Are all binaries in tools/telemetry/bin/linux/x86_64/* currently in the
public
> > bucket?
> 
> No, but they should be. I moved avconv to the public bucket so the bots could
> access it. Setting the bucket here just tells the cloud storage code to search
> the public bucket first, but if the binary is not in the public bucket it will
> search all other buckets as well.

Unless I'm missing something, I don't think that's true. It looks like multiple
buckets are only searched if the bucket param is not given.

  if bucket:
    buckets = [bucket]
  else:
    buckets = [PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET]

> 
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/video.py (left):
> 
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video.py:22: assert not
> video_file_obj.close_called
> On 2014/12/02 22:31:02, tonyg wrote:
> > Can you explain why these are no longer valid?
> 
> I didn't write this code, but these asserts made no sense. video_file_obj,
> according to the unit test for this code, is just a string, so it has no
delete
> or close_called methods.
> 
> Perhaps the unit test was just written poorly? Is there some actual video
object
> that has these methods?

Does git annotate have any clues? You're probably right that it should go away,
but it's usually not a good idea to remove an assert without understanding the
context in which it was added. Hopefully you can find the answer was that these
methods used to exist and no longer do (or something).

> 
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/video_unittest.py (right):
> 
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/video_unittest.py:22:
> host_platform.InstallApplication('avconv')
> On 2014/12/02 22:31:02, tonyg wrote:
> > Should the unittest really keep calling this? Seems like it should call some
> > actual Telemetry code that handles it.
> 
> I'm not sure I follow you. InstallApplication is the telemetry code that's
> supposed to handle the case that an application can't be launched, no?

The last I look at this code, all the installation was implicit. In other words,
when the appropriate objects were created or methods called, the installation
would happen. Telemetry needs to do this somewhere. So it seems odd to me that
the video_unittest, which should only be testing the Video class has to
explicitly install something.

[mthiesse, 2014-12-03 16:02:19]

On 2014/12/03 00:54:56, tonyg wrote:
> On 2014/12/02 23:23:26, mthiesse wrote:
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/platform/linux_platform_backend.py
> (right):
> > 
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/platform/linux_platform_backend.py:166:
> > cloud_storage.GetIfChanged(bin_path, cloud_storage.PUBLIC_BUCKET)
> > On 2014/12/02 22:31:02, tonyg wrote:
> > > Are all binaries in tools/telemetry/bin/linux/x86_64/* currently in the
> public
> > > bucket?
> > 
> > No, but they should be. I moved avconv to the public bucket so the bots
could
> > access it. Setting the bucket here just tells the cloud storage code to
search
> > the public bucket first, but if the binary is not in the public bucket it
will
> > search all other buckets as well.
> 
> Unless I'm missing something, I don't think that's true. It looks like
multiple
> buckets are only searched if the bucket param is not given.
> 
>   if bucket:
>     buckets = [bucket]
>   else:
>     buckets = [PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET]

Hmm you're correct. I tested switching this value to the public bucket before
re-uploading avconv to the public bucket, and it worked... but maybe avconv had
already been uploaded to the public bucket?

In any case, I think the correct solution here is to not pass the bucket
parameter, so it searches the public bucket first, then the other buckets.

> 
> > 
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/video.py (left):
> > 
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/video.py:22: assert not
> > video_file_obj.close_called
> > On 2014/12/02 22:31:02, tonyg wrote:
> > > Can you explain why these are no longer valid?
> > 
> > I didn't write this code, but these asserts made no sense. video_file_obj,
> > according to the unit test for this code, is just a string, so it has no
> delete
> > or close_called methods.
> > 
> > Perhaps the unit test was just written poorly? Is there some actual video
> object
> > that has these methods?
> 
> Does git annotate have any clues? You're probably right that it should go
away,
> but it's usually not a good idea to remove an assert without understanding the
> context in which it was added. Hopefully you can find the answer was that
these
> methods used to exist and no longer do (or something).

Okay, turns out it's just a bad test. The intent of the assert lines was to
ensure the passed object was a NamedTemporaryFile by checking for attributes
that a string doesn't have. I've re-written this logic in a more sensible way,
so that we can actually test the class without deleting the test data, and
support both temporary files and non-temporary files.

> 
> > 
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/core/video_unittest.py (right):
> > 
> >
>
https://codereview.chromium.org/756553003/diff/20001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/core/video_unittest.py:22:
> > host_platform.InstallApplication('avconv')
> > On 2014/12/02 22:31:02, tonyg wrote:
> > > Should the unittest really keep calling this? Seems like it should call
some
> > > actual Telemetry code that handles it.
> > 
> > I'm not sure I follow you. InstallApplication is the telemetry code that's
> > supposed to handle the case that an application can't be launched, no?
> 
> The last I look at this code, all the installation was implicit. In other
words,
> when the appropriate objects were created or methods called, the installation
> would happen. Telemetry needs to do this somewhere. So it seems odd to me that
> the video_unittest, which should only be testing the Video class has to
> explicitly install something.

Oh, I see what you mean. You're right, the test shouldn't be calling
InstallApplication, this was just another instance where the test was bad
(before it was *always* calling InstallApplication, without even testing if it
needed to).

[chromium-reviews, 2014-12-03 16:35:23]

Hmm, Tony, what's the reason for having avconv as an installable
dependency? avconv is already installed as part of install-build-deps, from
the package libav-tools. In fact, if the user does not have the libav-tools
package installed, downloading the avconv binary onto their system is
pointless because its dependencies aren't satisfied so it can't run.

On my system, if I uninstall avconv, then run the unit tests I get:
./bin/linux/x86_64/avconv: error while loading shared libraries:
libavfilter.so.2: cannot open shared object file: No such file or directory

If we wanted to install avconv in this way, we would also have to package
up its dependencies, which hasn't been done. Seeing as install-build-deps
already installs avconv, can I just remove avconv from InstallApplication?

On Wed, Dec 3, 2014 at 11:02 AM, <mthiesse@chromium.org> wrote:

> On 2014/12/03 00:54:56, tonyg wrote:
>
>> On 2014/12/02 23:23:26, mthiesse wrote:
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/platform/linux_platform_backend.py
>
>> > File tools/telemetry/telemetry/core/platform/linux_platform_backend.py
>> (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/platform/linux_platform_
> backend.py#newcode166
>
>> > tools/telemetry/telemetry/core/platform/linux_platform_backend.py:166:
>> > cloud_storage.GetIfChanged(bin_path, cloud_storage.PUBLIC_BUCKET)
>> > On 2014/12/02 22:31:02, tonyg wrote:
>> > > Are all binaries in tools/telemetry/bin/linux/x86_64/* currently in
>> the
>> public
>> > > bucket?
>> >
>> > No, but they should be. I moved avconv to the public bucket so the bots
>>
> could
>
>> > access it. Setting the bucket here just tells the cloud storage code to
>>
> search
>
>> > the public bucket first, but if the binary is not in the public bucket
>> it
>>
> will
>
>> > search all other buckets as well.
>>
>
>  Unless I'm missing something, I don't think that's true. It looks like
>>
> multiple
>
>> buckets are only searched if the bucket param is not given.
>>
>
>     if bucket:
>>      buckets = [bucket]
>>    else:
>>      buckets = [PUBLIC_BUCKET, PARTNER_BUCKET, INTERNAL_BUCKET]
>>
>
> Hmm you're correct. I tested switching this value to the public bucket
> before
> re-uploading avconv to the public bucket, and it worked... but maybe
> avconv had
> already been uploaded to the public bucket?
>
> In any case, I think the correct solution here is to not pass the bucket
> parameter, so it searches the public bucket first, then the other buckets.
>
>
>  >
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/video.py
>
>> > File tools/telemetry/telemetry/core/video.py (left):
>> >
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/video.py#oldcode22
>
>> > tools/telemetry/telemetry/core/video.py:22: assert not
>> > video_file_obj.close_called
>> > On 2014/12/02 22:31:02, tonyg wrote:
>> > > Can you explain why these are no longer valid?
>> >
>> > I didn't write this code, but these asserts made no sense.
>> video_file_obj,
>> > according to the unit test for this code, is just a string, so it has no
>> delete
>> > or close_called methods.
>> >
>> > Perhaps the unit test was just written poorly? Is there some actual
>> video
>> object
>> > that has these methods?
>>
>
>  Does git annotate have any clues? You're probably right that it should go
>>
> away,
>
>> but it's usually not a good idea to remove an assert without
>> understanding the
>> context in which it was added. Hopefully you can find the answer was that
>>
> these
>
>> methods used to exist and no longer do (or something).
>>
>
> Okay, turns out it's just a bad test. The intent of the assert lines was to
> ensure the passed object was a NamedTemporaryFile by checking for
> attributes
> that a string doesn't have. I've re-written this logic in a more sensible
> way,
> so that we can actually test the class without deleting the test data, and
> support both temporary files and non-temporary files.
>
>
>  >
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/video_unittest.py
>
>> > File tools/telemetry/telemetry/core/video_unittest.py (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/756553003/diff/20001/
> tools/telemetry/telemetry/core/video_unittest.py#newcode22
>
>> > tools/telemetry/telemetry/core/video_unittest.py:22:
>> > host_platform.InstallApplication('avconv')
>> > On 2014/12/02 22:31:02, tonyg wrote:
>> > > Should the unittest really keep calling this? Seems like it should
>> call
>>
> some
>
>> > > actual Telemetry code that handles it.
>> >
>> > I'm not sure I follow you. InstallApplication is the telemetry code
>> that's
>> > supposed to handle the case that an application can't be launched, no?
>>
>
>  The last I look at this code, all the installation was implicit. In other
>>
> words,
>
>> when the appropriate objects were created or methods called, the
>> installation
>> would happen. Telemetry needs to do this somewhere. So it seems odd to me
>> that
>> the video_unittest, which should only be testing the Video class has to
>> explicitly install something.
>>
>
> Oh, I see what you mean. You're right, the test shouldn't be calling
> InstallApplication, this was just another instance where the test was bad
> (before it was *always* calling InstallApplication, without even testing
> if it
> needed to).
>
> https://codereview.chromium.org/756553003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tonyg, 2014-12-03 19:36:24]

https://codereview.chromium.org/756553003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/756553003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:23: # Test if the input is a
NamedTemporaryFile, otherwise treat it as a string.
Why do we need to support both types? That's subtle and makes the code hard to
read. Could we keep the type assertion, but make it more explicit by relying on
type() or isinstance() or something?

[mthiesse, 2014-12-03 20:34:17]

https://codereview.chromium.org/756553003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/video.py (right):

https://codereview.chromium.org/756553003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/video.py:23: # Test if the input is a
NamedTemporaryFile, otherwise treat it as a string.
On 2014/12/03 19:36:24, tonyg wrote:
> Why do we need to support both types? That's subtle and makes the code hard to
> read. Could we keep the type assertion, but make it more explicit by relying
on
> type() or isinstance() or something?

Well, I think this was poorly architected to begin with. It makes no sense for
the class that reads video frames from a file to assume that its input is a
temporary file, and to be managing the lifecycle of the files passed to it. The
lifecycle of the file passed to this class should really be managed by the
calling class.

However, this isn't the case, and it looks like it would be non-trivial to fix
this, so I hope I've provided a reasonable compromise.

Requiring temporary file inputs is a big pain for unit testing, because you
can't create a NamedTemporaryFile from an existing file.

As for the code being 'subtle and hard to read', I think that's easily remedied
by adding the standard init method description, describing what's expected of
the input.

Using type() doesn't work because NamedTemporaryFile (_TemporaryFileWrapper) is
an old style python class, so type() returns <type 'instance'>. Using
isinstance() was rejected by the original author of this code because
NamedTemporaryFile isn't the class; the actual class is the private
_TemporaryFileWrapper.

[tonyg, 2014-12-04 01:25:09]

I agree the design is bad and that this patch shouldn't have to clean it up. But
I really don't want to hurt the API quality more by supporting a param of
multiple types. It looks to me like any file-like-object works as the parameter
(if we adjust the assertions). So could the test just do this:

with open(vid) as video_file:
  video_obj = video.Video(video_file)
  for i, timestamp ...
    ...

For further cleanups (not necessarily in this patch), I think we should move to
a model more like the Bitmap class where @staticmethods like FromPng(str) are
used to construct. Then those helpers can pass the right args to a more complex
ctor.

Anyway, if the above approach works for you, this should be good to go.

[mthiesse, 2014-12-04 15:38:48]

On 2014/12/04 01:25:09, tonyg wrote:
> I agree the design is bad and that this patch shouldn't have to clean it up.
But
> I really don't want to hurt the API quality more by supporting a param of
> multiple types. It looks to me like any file-like-object works as the
parameter
> (if we adjust the assertions). So could the test just do this:
> 
> with open(vid) as video_file:
>   video_obj = video.Video(video_file)
>   for i, timestamp ...
>     ...
> 
> For further cleanups (not necessarily in this patch), I think we should move
to
> a model more like the Bitmap class where @staticmethods like FromPng(str) are
> used to construct. Then those helpers can pass the right args to a more
complex
> ctor.
> 
> Anyway, if the above approach works for you, this should be good to go.

That's a good solution. Done.

[tonyg, 2014-12-04 17:34:29]

The CQ bit was checked by tonyg@chromium.org

[tonyg, 2014-12-04 17:34:29]

lgtm

[commit-bot: I haz the power, 2014-12-04 17:35:40]

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

[commit-bot: I haz the power, 2014-12-04 18:50:59]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-04 18:51:37]

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

[mfomitchev, 2014-12-05 15:13:21]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/748873003/ by mfomitchev@chromium.org.

The reason for reverting is: Looks like this broke telemetry_unittests
Build link:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/17356

Output:
[284/694] telemetry.core.video_unittest.VideoTest.testFramesFromMp4 failed
unexpectedly:
  Traceback (most recent call last):
    File
"/mnt/data/b/build/slave/Linux_Tests/build/src/tools/telemetry/telemetry/decorators.py",
line 84, in wrapper
      func(*args, **kwargs)
    File
"/mnt/data/b/build/slave/Linux_Tests/build/src/tools/telemetry/telemetry/core/video_unittest.py",
line 35, in testFramesFromMp4
      for i, timestamp_bitmap in enumerate(video_obj._FramesFromMp4()):
    File
"/mnt/data/b/build/slave/Linux_Tests/build/src/tools/telemetry/telemetry/core/video.py",
line 158, in _FramesFromMp4
      dimensions = GetDimensions(self._video_file_obj.name)
    File
"/mnt/data/b/build/slave/Linux_Tests/build/src/tools/telemetry/telemetry/core/video.py",
line 137, in GetDimensions
      output)
  AssertionError: Failed to determine video dimensions. output=avconv: error
while loading shared libraries: libavdevice.so.53: cannot open shared object
file: No such file or directory.