[libcamera-devel] [PATCH v3 2/2] tests: ipa: add tests to test IPAModule

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, May 20, 2019 at 08:40:59PM -0400, Paul Elder wrote:
> Add tests to test the the IPAModule class, for loading the IPA module
> info from IPA module .so shared objects, with modules written in both C
> and C++.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v3:
> - remove tests for incorrect bitness
> - make the test IPA module .so a C one and a C++ one
> 
> Changes in v2:
> - added source for test .so
> - updated tests to work with new (v2, see 1/2) IPAModule API
> 
>  test/ipa/ipa_test.cpp    | 65 ++++++++++++++++++++++++++++++++++++++++
>  test/ipa/meson.build     | 29 ++++++++++++++++++
>  test/ipa/shared_test.c   |  6 ++++
>  test/ipa/shared_test.cpp | 12 ++++++++
>  test/meson.build         |  1 +
>  5 files changed, 113 insertions(+)
>  create mode 100644 test/ipa/ipa_test.cpp
>  create mode 100644 test/ipa/meson.build
>  create mode 100644 test/ipa/shared_test.c
>  create mode 100644 test/ipa/shared_test.cpp
> 
> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp
> new file mode 100644
> index 0000000..74854df
> --- /dev/null
> +++ b/test/ipa/ipa_test.cpp
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * load-so.cpp - loading .so tests
> + */
> +
> +#include <iostream>
> +
> +#include "ipa_module.h"
> +
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +class IPAModuleTest : public Test
> +{
> +protected:
> +	int init()
> +	{
> +		return 0;
> +	}

No need to override init() if it's empty (same for cleanup() below).

> +
> +	int run_test(const string path)

s/run_test/runTest/

of better, name it testModuleValid() or similar ?

const string &path, there's no need to make a copy.

> +	{
> +		cout << "running lib loader test" << endl;
> +
> +		IPAModule *ll = new IPAModule(path);
> +
> +		if (!ll->isValid()) {
> +			cout << "failed to load" << endl;

	cout << "IPA module " << path << " is not valid" << endl;

You're leaking the ll pointer.

> +			return TestFail;
> +		}
> +
> +		struct IPAModuleInfo info = ll->info();
> +		cout << "loaded!" << endl;
> +		cout << "name = " << info.name << ", version = " << info.version << endl;

You should validate the information here instead of printing.

> +
> +		delete ll;
> +		return TestPass;
> +	}
> +
> +	int run()

	int run() override

> +	{
> +		int count = 0;
> +
> +		cout << "testing C IPAModule" << endl;

We usually don't print messages in tests other than failure messages.

> +		count += run_test("test/ipa/ipa-dummy.so");
> +
> +		cout << "testing C++ IPAModule" << endl;
> +		count += run_test("test/ipa/ipa-dummy-cpp.so");
> +
> +		if (count < 0)

This relies on TestPass being 0 and TestFail being -1, and on run_test()
not returning TestSkip. Is this safe ?

> +			return TestFail;
> +
> +		return TestPass;
> +	}
> +
> +	void cleanup()
> +	{
> +	}
> +};
> +
> +TEST_REGISTER(IPAModuleTest)
> diff --git a/test/ipa/meson.build b/test/ipa/meson.build
> new file mode 100644
> index 0000000..dc8fbdd
> --- /dev/null
> +++ b/test/ipa/meson.build
> @@ -0,0 +1,29 @@
> +ipa_test = [
> +    ['ipa_test',	      'ipa_test.cpp'],

meson.build files use spaces instead of tabs for indentation (here and
below).

> +]
> +
> +foreach t : ipa_test
> +    exe = executable(t[0], t[1],
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite: 'ipa', is_parallel: false)
> +endforeach
> +
> +ipa_dummy_sources = files([
> +    'shared_test.c'
> +])
> +
> +ipa_dummy_sources_cpp = files([
> +    'shared_test.cpp'
> +])
> +
> +ipa_dummy = shared_library('ipa-dummy',
> +			   ipa_dummy_sources,
> +			   name_prefix: '',
> +			   include_directories: test_includes_public)
> +
> +ipa_dummy_cpp = shared_library('ipa-dummy-cpp',
> +			       ipa_dummy_sources_cpp,
> +			       name_prefix: '',
> +			       include_directories: test_includes_public)

How about the (untested) following ?

ipa_modules = []

ipa_modules_sources = [
    [ipa_dummy_c,             files('shared_test.c')],
    [ipa_dummy_cpp,           files('shared_test.cpp')],
]

foreach m : ipa_modules_sources
    ipa_modules += shared_library(m[0], m[1], name_prefix: '',
                                  include_directories: test_includes_public)
endforeach

ipa_test = [
    ['ipa_test',              'ipa_test.cpp'],
]

foreach t : ipa_test
    exe = executable(t[0], t[1],
                     link_with : test_libraries,
                     include_directories : test_includes_internal)

    test(t[0], exe, suite: 'ipa', is_parallel: false)
endforeach

Ideally, we should specify the modules as dependencies for the tests:

    test(t[0], exe, suite: 'ipa', is_parallel: false,
         depends: ipa_modules)

but that was added in meson 0.46, which isn't available in Ubuntu 18.04
LTS, and I fear that would be an issue.

> diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c
> new file mode 100644
> index 0000000..4959a03
> --- /dev/null
> +++ b/test/ipa/shared_test.c
> @@ -0,0 +1,6 @@
> +#include <libcamera/ipa/ipa_module_info.h>
> +
> +const struct IPAModuleInfo ipaModuleInfo = {
> +	.name = "Answer to the Ultimate Question of Life, the Universe, and Everything",
> +	.version = 42,
> +};
> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp
> new file mode 100644
> index 0000000..4e5c976
> --- /dev/null
> +++ b/test/ipa/shared_test.cpp
> @@ -0,0 +1,12 @@
> +#include <libcamera/ipa/ipa_module_info.h>
> +
> +namespace libcamera {
> +
> +extern "C" {
> +const struct libcamera::IPAModuleInfo ipaModuleInfo = {
> +	"It's over nine thousand!",
> +	9001,
> +};
> +};
> +
> +}; /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index d501f2b..ef41367 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -1,6 +1,7 @@
>  subdir('libtest')
>  
>  subdir('camera')
> +subdir('ipa')
>  subdir('media_device')
>  subdir('pipeline')
>  subdir('v4l2_device')

-- 
Regards,

Laurent Pinchart