[PATCH v4 1/6] Added simple unit/integration test framework and corresponding test program.

[Pekka Paalanen]

On Tue, 23 Jun 2015 21:55:41 -0700
"Jon A. Cruz" <jonc at osg.samsung.com> wrote:

> 
> On 06/22/2015 07:19 AM, Pekka Paalanen wrote:
> > Hi Jon,
> > 
> > this is just a quick casual look, so I'm only mentioning what cought my
> > eye but mostly ignoring minor issues like style.
> > 
> > I know you sent a v5 series, but it sounded like my comments here are
> > still applicable.
> > 
> > I do get kind of confused every time a function is used earlier in the
> > file than where it is defined. I have a habit of scrolling up when I
> > see a function call I want to know what it does. Is that just me?
> 
> Looking into it more, I think I've figured out much of that on my part
> comes from C++ and other OO languages I've worked in. Especially with
> C++ one should start a class declaration with a single 'public' section
> followed optionally by a single 'protected' section and finally a single
> 'private' section if needed. So the general structure tends to be from
> higher-level first in the file down to lower-level later on. Also IDE
> use can promote the "F3 to go do declaration" over manually scrolling
> approach.
> 
> Since weston here is solidly in the "non-C++ C camp" I'll be reworking
> the order to match (either in later revisions or in follow-ups as needed).

Yeah. I could fix that for myself by switching to a better editor.

But redundant declarations are extra code to be kept in sync.

> >> +/**
> >> + * Creates an instance of the test fixture that is chained.
> >> + * Additional registry advertisements will be passed on.
> >> + *
> >> + * @param display name of the wayland display to connect to or NULL for
> >> + * the default.
> >> + * @param reg_listener the registry listener to chain in.
> >> + * @param data pointer to pass back to registry listener.
> >> + * @return a pointer to a test fixture upon success, NULL otherwise.
> >> + * @see wtst_ctx_destroy()
> >> + */
> >> +struct wtst_ctx *
> >> +wtst_ctx_create_chained(const char *display,
> >> +			struct wl_registry_listener *reg_listener,
> >> +			void *reg_data);
> >> +
> >> +/**
> >> + * Creates an instance of the test fixture that is chained and with a
> >> + * corresponding surface.
> >> + * Additional registry advertisements will be passed on.
> >> + *
> >> + * @param width the width for the surface.
> >> + * @param height the height for the surface.
> >> + * @param reg_listener the registry listener to chain in.
> >> + * @param data pointer to pass back to registry listener.
> >> + * @return a pointer to a test fixture upon success, NULL otherwise.
> >> + * @see wtst_ctx_destroy()
> >> + */
> >> +struct wtst_ctx *
> >> +wtst_ctx_create_with_surface_chained(const char *display,
> >> +				     int width, int height,
> >> +				     struct wl_registry_listener *reg_listener,
> >> +				     void *reg_data);
> > 
> > What does "chained" mean? Where does the word come from?
> > 
> > Is it just for tests that want to touch a specific global the fixture
> > does not handle?
> 
> In general, yes. It also allows fixtures to be layered up so that you
> can have a Weston fixture that adds onto a generic Wayland fixture.
> Things like that.
> 
> Definitely sounds like I need to beef up that comment and make intent
> clearer.

An example on how that chaining works would be enlightening. I'm not
sure there was one in this series...

> >> +/**
> >> + * Trigger specific failure/signal upon test failures; useful when
> >> + * running under a debugger.
> >> + *
> >> + * @param break_on_failure true to cause a break when tests fail, false to
> >> + * allow normal operation upon failures.
> >> + */
> >> +void
> >> +zuc_set_break_on_failure(bool break_on_failure);
> > 
> > What does the break do exactly?
> 
> It probably does not do as much as it should currently. However it does
> cause gdb to catch a stop in execution. Some frameworks take it a step
> further and actually invoke the proper debugger.

I guessed it might raise(SIGTRAP) or such. Documenting that would be
nice.

> >> +void
> >> +zuc_set_spawn(bool spawn);
> > 
> > There are lots of zuc_set_*() functions. What are the default settings?
> 
> Should it just document the defaults? (which, of course, will make me
> commit to their behavior). And/or would these need gettors?

Docs, please.


> >> +/**
> >> + * Terminates the current test without marking it as failed.
> >> + *
> >> + * @param message the message to log as to why the test has been skipped.
> >> + */
> >> +#define ZUC_SKIP(message) \
> >> +	do { \
> >> +		zucimpl_terminate(__FILE__, __LINE__, false, false, #message); \
> >> +		return; \
> >> +	} \
> >> +	while (0)
> > 
> > Hmm, that "return". That needs extra documentation so that people don't
> > call it from a sub-function without checking for skips in the caller.
> > It also makes the assumption of the return type being void.
> 
> Yes. This is where usage starts to get a little tricky.
> 
> Translating a common idiom from gtest we could have
> 
>     some_function_that_checks();
>     ZUC_ASSERT(!zuc_has_failure());
> 
> I've also been pondering adding a parallel set that is non-void, similar
> to GTK+/GLib's g_return_val_if_fail()

Both sound ok.


> >> +/**
> >> + * Reads events from the given pipe and processes them.
> >> + *
> >> + * @param test the currently active test to attache events for.
> >> + * @param pipe_fd the file descriptor of the pipe to read from.
> >> + * @return a positive value if a message was received, 0 if the end has
> >> + * been reached and -1 if an error has occurred.
> >> + */
> >> +int
> >> +zuc_process_message(struct zuc_test *test, int pipe_fd);
> > 
> > Ah, message passing between processes. Nice. I suppose if you fork(),
> > and you want structured output like those xml stuffs, nothing simpler
> > will really cut it?
> 
> Correct. I've coded mechanisms, used different libs with them... many
> different ways. Definitiely in the simpler-is-better camp here. Getting
> things so that the fork() will work, but is still optional, and behaves
> in ways not to confuse valgrind was the main target.
> 
> I definitely don't want to hook in SOAP here or anything (and, yes, I
> have had that pushed by architects before in such scenarios)

Nice that we agree. :-)

> >> +/*
> >> + * If CLOCK_MONOTONIC is present on the system it will give us reliable
> >> + * results under certain edge conditions that normally require manual
> >> + * admin actions to trigger. If not, CLOCK_REALTIME is a reasonable
> >> + * fallback.
> >> + */
> >> +#if _POSIX_MONOTONIC_CLOCK
> >> +static const clockid_t TARGET_TIMER = CLOCK_MONOTONIC;
> >> +#else
> >> +static const clockid_t TARGET_TIMER = CLOCK_REALTIME;
> >> +#endif
> > 
> > I do wonder if build time checks are appropriate. I did a runtime check
> > for weston's presentation clock. Not that it would matter here.
> 
> From some general experience here it seems POSIX makes for a fair
> compile time check. Especially if one favors compile-time safety over
> run-time.

I don't really find it too different to test whether
_POSIX_MONOTONIC_CLOCK or CLOCK_MONOTONIC are #defined.

If CLOCK_MONOTONIC is not defined, we could even define it ourselves.
The main question is, does the runtime support it.


> >> +bool
> >> +wildcard_matches(char const *wildcard, char const *str)
> >> +{
> >> +	switch (*wildcard) {
> >> +	case '\0':
> >> +		return !*str;
> >> +	case '?':
> >> +		return *str && wildcard_matches(wildcard + 1, str + 1);
> >> +	case '*':
> >> +		return (*str && wildcard_matches(wildcard, str + 1))
> >> +			|| wildcard_matches(wildcard + 1, str);
> >> +	default:
> >> +		return (*wildcard == *str)
> >> +			&& wildcard_matches(wildcard + 1, str + 1);
> >> +	};
> >> +}
> > 
> > Umm, I saw somewhere you #included fnmatch.h. Why not use fnmatch()?
> 
> I'll have to check if that is still needed (might not be). I'd started
> with fnmatch(), but the behavior does not match the more common unit
> test conventions. I'd also checked using regcomp() and at least one other.

The function is pretty simple looking, but it recurses by string
length. That's my main worry, if the semantics are as intended.


Thanks,
pq