[libcamera-devel] [PATCH] tests: Add a base Test class

[Laurent Pinchart]

Hi Kieran,

On Thursday, 20 December 2018 21:40:52 EET Kieran Bingham wrote:
> On 20/12/2018 18:53, Niklas Söderlund wrote:
> > On 2018-12-19 11:48:30 +0200, Laurent Pinchart wrote:
> >> The base Test class is meant to provide infrastructure common to all
> >> tests. It is very limited for now, and should be extended with at least
> >> logging and assertion handling.
> > 
> > I agree it's a bit limited now but can serve as a good base to build
> > upon. I have used this to write tests for device enumerator work.
> 
> I've also used this as a base to rework my tests on top of, and I've now
> refactored my v4l2device tests which does indeed reduce quite a bit of
> code duplication.
> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > 
> >> ---
> >> 
> >>  test/meson.build |  6 ++++++
> >>  test/test.cpp    | 28 ++++++++++++++++++++++++++++
> >>  test/test.h      | 28 ++++++++++++++++++++++++++++
> >>  3 files changed, 62 insertions(+)
> >>  create mode 100644 test/test.cpp
> >>  create mode 100644 test/test.h
> >> 
> >> diff --git a/test/meson.build b/test/meson.build
> >> index 24d1927f977c..fc9c124927d2 100644
> >> --- a/test/meson.build
> >> +++ b/test/meson.build
> >> @@ -1,3 +1,9 @@
> >> +libtest_sources = files([
> >> +    'test.cpp',
> >> +])
> >> +
> >> +libtest = static_library('libtest', libtest_sources)
> 
> I wonder if libtest should get it's own directory as test/libtest to
> separate it out. Especially as it will likely expand with logging and
> other helpers.

That would make sense. Or maybe moving tests in subdirectories. I think we can 
decide when we'll get enough source files to call for a move.

> >> +
> >>  test_init = executable('test_init', 'init.cpp',
> >>                         link_with : libcamera,
> >>                         include_directories : libcamera_includes)
> >> diff --git a/test/test.cpp b/test/test.cpp
> >> new file mode 100644
> >> index 000000000000..4e7779e750d5
> >> --- /dev/null
> >> +++ b/test/test.cpp
> >> @@ -0,0 +1,28 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * test.cpp - libcamera test base class
> >> + */
> >> +
> >> +#include "test.h"
> >> +
> >> +Test::Test()
> >> +{
> >> +}
> >> +
> >> +Test::~Test()
> >> +{
> >> +	cleanup();
> >> +}
> >> +
> >> +int Test::execute()
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = init();
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	return run();
> >> +}
> >> diff --git a/test/test.h b/test/test.h
> >> new file mode 100644
> >> index 000000000000..2464fc5cb607
> >> --- /dev/null
> >> +++ b/test/test.h
> >> @@ -0,0 +1,28 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * test.h - libcamera test base class
> >> + */
> 
> Shouldn't we have an include guard here?

It should :-) Fixed and pushed.

> >> +
> >> +#include <sstream>
> >> +
> >> +class Test
> >> +{
> >> +public:
> >> +	Test();
> >> +	virtual ~Test();
> >> +
> >> +	int execute();
> >> +
> >> +protected:
> >> +	virtual int init() { return 0; }
> >> +	virtual int run() = 0;
> >> +	virtual void cleanup() { }
> >> +};
> >> +
> >> +#define TEST_REGISTER(klass)						\
> >> +int main(int argc, char *argv[])					\
> >> +{									\
> >> +	return klass().execute();					\
> >> +}

-- 
Regards,

Laurent Pinchart