[{"id":1646,"web_url":"https://patchwork.libcamera.org/comment/1646/","msgid":"<20190521151103.GI5674@pendragon.ideasonboard.com>","date":"2019-05-21T15:11:03","subject":"Re: [libcamera-devel] [PATCH v3 2/2] tests: ipa: add tests to test\n\tIPAModule","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, May 20, 2019 at 08:40:59PM -0400, Paul Elder wrote:\n> Add tests to test the the IPAModule class, for loading the IPA module\n> info from IPA module .so shared objects, with modules written in both C\n> and C++.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v3:\n> - remove tests for incorrect bitness\n> - make the test IPA module .so a C one and a C++ one\n> \n> Changes in v2:\n> - added source for test .so\n> - updated tests to work with new (v2, see 1/2) IPAModule API\n> \n> test/ipa/ipa_test.cpp | 65 ++++++++++++++++++++++++++++++++++++++++\n> test/ipa/meson.build | 29 ++++++++++++++++++\n> test/ipa/shared_test.c | 6 ++++\n> test/ipa/shared_test.cpp | 12 ++++++++\n> test/meson.build | 1 +\n> 5 files changed, 113 insertions(+)\n> create mode 100644 test/ipa/ipa_test.cpp\n> create mode 100644 test/ipa/meson.build\n> create mode 100644 test/ipa/shared_test.c\n> create mode 100644 test/ipa/shared_test.cpp\n> \n> diff --git a/test/ipa/ipa_test.cpp b/test/ipa/ipa_test.cpp\n> new file mode 100644\n> index 0000000..74854df\n> --- /dev/null\n> +++ b/test/ipa/ipa_test.cpp\n> @@ -0,0 +1,65 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * load-so.cpp - loading .so tests\n> + */\n> +\n> +#include \n> +\n> +#include \"ipa_module.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class IPAModuleTest : public Test\n> +{\n> +protected:\n> +\tint init()\n> +\t{\n> +\t\treturn 0;\n> +\t}\n\nNo need to override init() if it's empty (same for cleanup() below).\n\n> +\n> +\tint run_test(const string path)\n\ns/run_test/runTest/\n\nof better, name it testModuleValid() or similar ?\n\nconst string &path, there's no need to make a copy.\n\n> +\t{\n> +\t\tcout << \"running lib loader test\" << endl;\n> +\n> +\t\tIPAModule *ll = new IPAModule(path);\n> +\n> +\t\tif (!ll->isValid()) {\n> +\t\t\tcout << \"failed to load\" << endl;\n\n\tcout << \"IPA module \" << path << \" is not valid\" << endl;\n\nYou're leaking the ll pointer.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tstruct IPAModuleInfo info = ll->info();\n> +\t\tcout << \"loaded!\" << endl;\n> +\t\tcout << \"name = \" << info.name << \", version = \" << info.version << endl;\n\nYou should validate the information here instead of printing.\n\n> +\n> +\t\tdelete ll;\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint run()\n\n\tint run() override\n\n> +\t{\n> +\t\tint count = 0;\n> +\n> +\t\tcout << \"testing C IPAModule\" << endl;\n\nWe usually don't print messages in tests other than failure messages.\n\n> +\t\tcount += run_test(\"test/ipa/ipa-dummy.so\");\n> +\n> +\t\tcout << \"testing C++ IPAModule\" << endl;\n> +\t\tcount += run_test(\"test/ipa/ipa-dummy-cpp.so\");\n> +\n> +\t\tif (count < 0)\n\nThis relies on TestPass being 0 and TestFail being -1, and on run_test()\nnot returning TestSkip. Is this safe ?\n\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tvoid cleanup()\n> +\t{\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(IPAModuleTest)\n> diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> new file mode 100644\n> index 0000000..dc8fbdd\n> --- /dev/null\n> +++ b/test/ipa/meson.build\n> @@ -0,0 +1,29 @@\n> +ipa_test = [\n> + ['ipa_test',\t 'ipa_test.cpp'],\n\nmeson.build files use spaces instead of tabs for indentation (here and\nbelow).\n\n> +]\n> +\n> +foreach t : ipa_test\n> + exe = executable(t[0], t[1],\n> + link_with : test_libraries,\n> + include_directories : test_includes_internal)\n> +\n> + test(t[0], exe, suite: 'ipa', is_parallel: false)\n> +endforeach\n> +\n> +ipa_dummy_sources = files([\n> + 'shared_test.c'\n> +])\n> +\n> +ipa_dummy_sources_cpp = files([\n> + 'shared_test.cpp'\n> +])\n> +\n> +ipa_dummy = shared_library('ipa-dummy',\n> +\t\t\t ipa_dummy_sources,\n> +\t\t\t name_prefix: '',\n> +\t\t\t include_directories: test_includes_public)\n> +\n> +ipa_dummy_cpp = shared_library('ipa-dummy-cpp',\n> +\t\t\t ipa_dummy_sources_cpp,\n> +\t\t\t name_prefix: '',\n> +\t\t\t include_directories: test_includes_public)\n\nHow about the (untested) following ?\n\nipa_modules = []\n\nipa_modules_sources = [\n [ipa_dummy_c, files('shared_test.c')],\n [ipa_dummy_cpp, files('shared_test.cpp')],\n]\n\nforeach m : ipa_modules_sources\n ipa_modules += shared_library(m[0], m[1], name_prefix: '',\n include_directories: test_includes_public)\nendforeach\n\nipa_test = [\n ['ipa_test', 'ipa_test.cpp'],\n]\n\nforeach t : ipa_test\n exe = executable(t[0], t[1],\n link_with : test_libraries,\n include_directories : test_includes_internal)\n\n test(t[0], exe, suite: 'ipa', is_parallel: false)\nendforeach\n\nIdeally, we should specify the modules as dependencies for the tests:\n\n test(t[0], exe, suite: 'ipa', is_parallel: false,\n depends: ipa_modules)\n\nbut that was added in meson 0.46, which isn't available in Ubuntu 18.04\nLTS, and I fear that would be an issue.\n\n> diff --git a/test/ipa/shared_test.c b/test/ipa/shared_test.c\n> new file mode 100644\n> index 0000000..4959a03\n> --- /dev/null\n> +++ b/test/ipa/shared_test.c\n> @@ -0,0 +1,6 @@\n> +#include \n> +\n> +const struct IPAModuleInfo ipaModuleInfo = {\n> +\t.name = \"Answer to the Ultimate Question of Life, the Universe, and Everything\",\n> +\t.version = 42,\n> +};\n> diff --git a/test/ipa/shared_test.cpp b/test/ipa/shared_test.cpp\n> new file mode 100644\n> index 0000000..4e5c976\n> --- /dev/null\n> +++ b/test/ipa/shared_test.cpp\n> @@ -0,0 +1,12 @@\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +extern \"C\" {\n> +const struct libcamera::IPAModuleInfo ipaModuleInfo = {\n> +\t\"It's over nine thousand!\",\n> +\t9001,\n> +};\n> +};\n> +\n> +}; /* namespace libcamera */\n> diff --git a/test/meson.build b/test/meson.build\n> index d501f2b..ef41367 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -1,6 +1,7 @@\n> subdir('libtest')\n> \n> subdir('camera')\n> +subdir('ipa')\n> subdir('media_device')\n> subdir('pipeline')\n> subdir('v4l2_device')","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8E1360C27\n\tfor ;\n\tTue, 21 May 2019 17:11:20 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 176CC52C;\n\tTue, 21 May 2019 17:11:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1558451480;\n\tbh=K/E3Bzs3Q5PTsOT2X0et0+UQ0gfX3zyl0R5i1KVpGTs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l3rM/GnzD37qkHl0aADHS7XKwt5XmTm4kSzmV5NZz1V9nB788NRDX4GctIFN+vf/k\n\tlJrMLSer1EMWXPnTD+cS1ROYKU+cIs8Vgd6L9AYkvnJM1jFbJKA9F9CX8BUVgnt4qB\n\tMPNSQtQwYmV8cCCbDiZWQPiDrgX3eJp0Tk+472DA=","Date":"Tue, 21 May 2019 18:11:03 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190521151103.GI5674@pendragon.ideasonboard.com>","References":"<20190521004059.7750-1-paul.elder@ideasonboard.com>\n\t<20190521004059.7750-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190521004059.7750-2-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 2/2] tests: ipa: add tests to test\n\tIPAModule","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 21 May 2019 15:11:20 -0000"}}]