[Bug 751916] Add GstHarness test framework

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=751916

--- Comment #4 from Håvard Graff (hgr) <havard.graff at gmail.com> ---
(In reply to Tim-Philipp Müller from comment #2)

Comments inline: 

> Some quick comments/notes/questions/nitpicks:
> 
>  - config.h include in installed header file needs to move to .c file
Done!

> 
>  - does struct _GstHarness need to be public? Should be made private IMHO
If this was a "normal" implementation, I would absolutely agree, however, since
this is code meant for testing, and testing only, I often find it very useful
to be able to access the internals with the higher goal of writing better
tests. The best example of this is actually the sub-harnesses, where we often
do what you suggest further down, in nesting the harnesses, and being able to
access them. Very common pattern in many of our tests is adding a launch-line
as a src-harness (gst_harness_add_src_parse) and then use
gst_harness_find_element on h->src_harness in order to access an element inside
the src_harness

> 
>  - naming consistency:
>    gst_harness_add_element_srcpad/sinkpad() vs. set_src_caps/sink_caps()
Done!

> 
>  - some functions take ownership of arguments passed, this needs to be
>    documented with the appropriate transfer annotation in the doc chunk
>    (e.g. gst_harness_set_sink_caps)
Done!
> 
>  - confusing terminology: (basically caused by leaking implementation
> details into the API?):
>     - gst_harness_set_src_caps() = input caps, and _set_sink_caps() = output
> caps
>     - gst_harness_add_src() etc. [wonder if "input" would generally be
> nicer?]
>     - e.g. "Pulls a #GstBuffer from the #GAsyncQueue on the #GstHarness
> sinkpad"
So this is not confusing to me, having used this now for 3 years, but I do see
your point. The harness has a srcpad and sinkpad, and being able to define
their caps is important. Also with the harnesses, you are effectively adding an
"alternative" src-pad for your harness with a src-harness, making the naming
consistent with GStreamer uses for src and sink. 

> 
>  - example in the docs section at the beginning: not even a _play() required?
>    what is _play for then?
RTFM :) I think I documented that quite well in the docstring for
gst_harness_play? 

> 
>  - gst_harness_set_pull_mode():
>     - docs typo: releasing->release
>     - unfortunate naming, since it has nothing to do with GStreamer pull mode
Agree! New name is _set_blocking_push_mode! Thanks!
> 
>  - gst_harness_try_pull() - why doesn't this also signal on the pull_cond
>    if pull_mode_active, just like _pull() does?
No reason. Fixed!

> 
>  - gst_harness_buffers_received() should do atomic int get;
>    also better describe difference to buffers_in_queue in docs
>    (received is total so far?); mention that it includes dropped buffers
>    - same for _events_received() and _upstream_events_received()
Done!

> 
>  - gst_harness_set_us_latency() - why 'us', isn't it in nanoseconds?
us = upstream. Fixed.

> 
>  - now the idea of a 'src harness' is baked into the API, but internal to
>    the harness, I wonder if it would make sense to basically allow setting
>    of a src GstHarness instead, and then just call stuff on that?
>    (i.e. make the whole thing nestable ultimately) (guess that could always
>    be added later if needed)
See comment above.

> 
>  - gst_harness_signal() doesn't seem to make any sense?
>     - can't pass arguments or retrieve return values
>     - should be renamed to emit_signal_by_name()
Agree, this is messy. Removed _signal and _signal_connect until a better
implementation can be done.

> 
>  - _add_probe() gets destroy_notify for user_data but _connect_signal() not?
See comment above. Now gone.

> 
>  - the comment above gst_harness_stress_push_event_start_full() in the header
>    should be in the gtk-doc chunk
Done!

> 
>  - stress tests: not sure I like the timeout in G_USECs
Have not done anything about this. Can we live with it?

> 
>  - stress tests: multiple stress tests could theoretically be run in parallel
>    now, corrct? (I say 'now' because I don't thin that was the case last time
>    I looked at the API)
It is indeed!

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.