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

61 diff mbox series

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 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

Hi Laurent,

Thanks for your work.

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.

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@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)
> +
>  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
> + */
> +
> +#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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On 20/12/2018 18:53, Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your work.
> 
> 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@ideasonboard.com>
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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.

>> +
>>  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?

>> +
>> +#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
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>

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@ideasonboard.com>
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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();					\
> >> +}