GNOME Bugzilla – Bug 751916
Add GstHarness test framework
Last modified: 2015-08-16 13:37:23 UTC
Created attachment 306733 [details] [review] Patch with GstHarness https://www.youtube.com/watch?v=uHf_sZ8qxck
One quick note, you need to add this to the doc (*-section.txt).
Some quick comments/notes/questions/nitpicks: - config.h include in installed header file needs to move to .c file - does struct _GstHarness need to be public? Should be made private IMHO - naming consistency: gst_harness_add_element_srcpad/sinkpad() vs. set_src_caps/sink_caps() - 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) - 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" - example in the docs section at the beginning: not even a _play() required? what is _play for then? - gst_harness_set_pull_mode(): - docs typo: releasing->release - unfortunate naming, since it has nothing to do with GStreamer pull mode - gst_harness_try_pull() - why doesn't this also signal on the pull_cond if pull_mode_active, just like _pull() does? - 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() - gst_harness_set_us_latency() - why 'us', isn't it in nanoseconds? - 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) - 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() - _add_probe() gets destroy_notify for user_data but _connect_signal() not? - the comment above gst_harness_stress_push_event_start_full() in the header should be in the gtk-doc chunk - stress tests: not sure I like the timeout in G_USECs - 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)
Created attachment 306922 [details] [review] Patch with GstHarness
(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!
> > - 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 I get that, but it means it's hard to change implementation details later. I guess you have more confidence than me in the details given you've been using this a bit longer.. Are you mainly accessing the sub-harnesses, or other things too? I was actually wondering if the sub-harness creation function should perhaps return a pointer to the sub-harness. > > - 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. Yeah, I get that of course. And I'm fine with it, but I think these details need to be documented/described somewhere then (or maybe they are and I missed it). > > - 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? Fair enough - by "src elements" you mean source elements in general, or src sub-harnesses? > > - stress tests: not sure I like the timeout in G_USECs > Have not done anything about this. Can we live with it? We can! Other than that it looks generally fine to me and I think we should just get it in. Seeing that you've got a few tests using that it probably mostly works.
Created attachment 306960 [details] [review] Patch with GstHarness This is the updated GstHarness patch.
commit ac79c7f05a099bfefb6dced192165a3e2caf6862 Author: Tim-Philipp Müller <tim@centricular.com> Date: Tue Jul 7 00:56:41 2015 +0100 harness: rename GstHarnessPrepareBuffer -> GstHarnessPrepareBufferFunc https://bugzilla.gnome.org/show_bug.cgi?id=751916 commit 49c896db3ae9290b3358979c611d12bd0501d805 Author: Tim-Philipp Müller <tim@centricular.com> Date: Tue Jul 7 00:53:48 2015 +0100 docs: add GstHarness to documentation https://bugzilla.gnome.org/show_bug.cgi?id=751916 commit 66e25da31327704cece47a8aa7b2819b609703ad Author: Havard Graff <havard.graff@gmail.com> Date: Mon Dec 16 10:47:47 2013 +0100 check: Add GstHarness convenience API for unit tests http://gstconf.ubicast.tv/videos/gstharness-again-a-follow-up/ https://bugzilla.gnome.org/show_bug.cgi?id=751916
*** Bug 728368 has been marked as a duplicate of this bug. ***