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

1239 diff mbox series

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@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

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@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')