After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747100 - tests: filesink: add test for GstFileSink render_list implemention
tests: filesink: add test for GstFileSink render_list implemention
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-31 10:21 UTC by Prashant Gotarne
Modified: 2015-04-01 11:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
filesink: test_seeking test is updated to verify the implementation of virtual method render_list (3.28 KB, patch)
2015-03-31 10:23 UTC, Prashant Gotarne
none Details | Review
tests: filesink: added checks for render_list virtual method (3.25 KB, patch)
2015-04-01 03:49 UTC, Prashant Gotarne
committed Details | Review

Description Prashant Gotarne 2015-03-31 10:21:56 UTC
Test suite for the GstFileSink element does not check the functionality of virtual method render_list implemented by the GstFileSink. 
Need a testcase to verify that the implemented method for render_list by GstFileSink element works correctly.
Comment 1 Prashant Gotarne 2015-03-31 10:23:01 UTC
Created attachment 300655 [details] [review]
filesink: test_seeking test is updated to verify the implementation of virtual method render_list

Please verify the patch.
Comment 2 Tim-Philipp Müller 2015-03-31 18:41:50 UTC
Comment on attachment 300655 [details] [review]
filesink: test_seeking test is updated to verify the implementation of virtual method render_list

Thanks a lot for the patch. Better unit test coverage is always welcome.

Two small nitpicks:

>Subject: [PATCH] filesink: test_seeking test is updated to verify the
> implementation of virtual method render_list

This 'summary line' is quite long, try to make it as short and concise as possible, e.g. "tests: filesink: add check for render_list virtual method" or similar.

>GstFileSink implements render_list vitual method to render list of buffers. There is no method present till now in filesink testsuit to check the functionality of this virtual method implementation by GstFileSink.
>Updating the test_seeking testcase to check render_list method implementation where it push the buffer_list to the pad and verify the position to make sure it is successfully written to file.

It's great that you took the time to write a detailed commit message, but this might be a bit too much detail really. You usualy don't have to justify why you add a new unit test, it's self-evident why it's a good thing. Please wrap the commit message lines (not the summary line) at ca. ~80 characters or so.

About the code: it looks good, but it would be great if we could also check the actual data written out if it's not too much extra work (there seems to be a macro for that already).
Comment 3 Prashant Gotarne 2015-04-01 03:49:01 UTC
Created attachment 300723 [details] [review]
tests: filesink: added checks for render_list virtual method

Updated patch with changes specified in review comments.
Comment 4 Tim-Philipp Müller 2015-04-01 11:34:34 UTC
Thanks, pushed:

commit 714d8e58e35cacaeab519ea0970d9a9147ee9dc1
Author: Prashant Gotarne <ps.gotarne@samsung.com>
Date:   Wed Apr 1 09:20:24 2015 +0530

    tests: filesink: add check for render_list virtual method
    
    GstFileSink implements the render_list virtual method to render
    a list of buffers. Update the test_seeking test case to also
    check the render_list method implementation.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=747100


For what it's worth, there is also the case of a buffer with multiple memories and a buffer list with buffers that each have one or more memories :)