[{"id":16962,"web_url":"https://patchwork.libcamera.org/comment/16962/","msgid":"","date":"2021-05-16T14:06:39","subject":"Re: [libcamera-devel] [PATCH v3 4/4] lc-compliance: Add list and\n\tfilter parameters","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Nícolas,\n\nThanks for your work.\n\nOn 2021-05-14 10:16:52 -0300, Nícolas F. R. A. Prado wrote:\n> Add a --list parameter that lists all current tests (by mapping to\n> googletest's --gtest_list_tests).\n> \n> Add a --filter 'filterString' parameter that filters the tests to run\n> (by mapping to googletest's --gtest_filter).\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/lc-compliance/main.cpp | 68 ++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 66 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index d5ff93f514df..bebf1c51f64b 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -20,6 +20,8 @@ using namespace libcamera;\n> \n> enum {\n> \tOptCamera = 'c',\n> +\tOptList = 'l',\n> +\tOptFilter = 'f',\n> \tOptHelp = 'h',\n> };\n> \n> @@ -54,16 +56,26 @@ public:\n> \t~Harness();\n> \n> \tint init();\n> +\tint buildGtestParameters(char *arg0);\n> +\tint gtestArgc() { return gtestArgc_; };\n> +\tchar **gtestArgv() { return gtestArgv_; };\n\nThis feels a bit like a layer violation.\n\nDo you think it would make sens to instead move the gtest data \nstructures inside the Harness class? Or perhaps the Harness class is not \nas useful now that it likely won't grow to support all the test \ninfrastructure provided by gtest and should instead be removed and what \nis left of it moved to main() and such helpers?\n\n> \n> private:\n> \tvoid listCameras();\n> \n> \tOptionsParser::Options options_;\n> \tstd::unique_ptr cm_;\n> +\n> +\tconst std::map gtestFlag_ = {{\"list\", \"--gtest_list_tests\"},\n> +\t\t\t\t\t\t\t {\"filter\", \"--gtest_filter\"}};\n> +\n> +\tint gtestArgc_;\n> +\tchar **gtestArgv_;\n> +\tstd::string gtestFilterParam_;\n> };\n> \n> Harness::Harness(const OptionsParser::Options &options)\n> -\t: options_(options)\n> +\t: options_(options), gtestArgv_(nullptr)\n> {\n> \tcm_ = std::make_unique();\n> }\n> @@ -73,6 +85,8 @@ Harness::~Harness()\n> \tEnvironment::instance()->destroy();\n> \n> \tcm_->stop();\n> +\n> +\tfree(gtestArgv_);\n> }\n> \n> int Harness::init()\n> @@ -86,6 +100,9 @@ int Harness::init()\n> \t\treturn ret;\n> \t}\n> \n> +\tif (options_.isSet(OptList))\n> +\t\treturn 0;\n> +\n> \tif (!options_.isSet(OptCamera)) {\n> \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> \t\tlistCameras();\n> @@ -118,12 +135,54 @@ void Harness::listCameras()\n> \t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> }\n> \n> +int Harness::buildGtestParameters(char *arg0)\n> +{\n> +\tint argc = 0;\n> +\n> +\t/*\n> +\t * +2 to have space for both the 0th argument that is needed but not\n> +\t * used and the null at the end.\n> +\t */\n> +\tgtestArgv_ = (char**) malloc((gtestFlag_.size() + 2) * sizeof(char*));\n> +\tif (!gtestArgv_)\n> +\t\treturn -ENOMEM;\n> +\n> +\tgtestArgv_[argc] = arg0;\n> +\targc++;\n> +\n> +\tif (options_.isSet(OptList)) {\n> +\t\tgtestArgv_[argc] = const_cast(gtestFlag_.at(\"list\").c_str());\n> +\t\targc++;\n> +\t}\n> +\n> +\tif (options_.isSet(OptFilter)) {\n> +\t\t/*\n> +\t\t * The filter flag needs to be passed as a single parameter, in\n> +\t\t * the format --gtest_filter=filterStr\n> +\t\t */\n> +\t\tconst std::string &filter = options_[OptFilter];\n> +\t\tgtestFilterParam_ = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> +\n> +\t\tgtestArgv_[argc] = const_cast(gtestFilterParam_.c_str());\n> +\t\targc++;\n> +\t}\n> +\n> +\tgtestArgv_[argc] = 0;\n> +\tgtestArgc_ = argc;\n> +\n> +\treturn 0;\n> +}\n> +\n> static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> {\n> \tOptionsParser parser;\n> \tparser.addOption(OptCamera, OptionString,\n> \t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n> \t\t\t ArgumentRequired, \"camera\");\n> +\tparser.addOption(OptList, OptionNone, \"List all tests and exit\", \"list\");\n> +\tparser.addOption(OptFilter, OptionString,\n> +\t\t\t \"Specify which tests to run\", \"filter\",\n> +\t\t\t ArgumentRequired, \"filter\");\n> \tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n> \t\t\t \"help\");\n> \n> @@ -170,7 +229,12 @@ int main(int argc, char **argv)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\t::testing::InitGoogleTest(&argc, argv);\n> +\tret = harness.buildGtestParameters(argv[0]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\tint gtestArgc = harness.gtestArgc();\n> +\n> +\t::testing::InitGoogleTest(>estArgc, harness.gtestArgv());\n> \ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> \treturn RUN_ALL_TESTS();\n> }\n> -- \n> 2.31.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92F8BC31FC\n\tfor ;\n\tSun, 16 May 2021 14:06:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 094056891F;\n\tSun, 16 May 2021 16:06:43 +0200 (CEST)","from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com\n\t[IPv6:2a00:1450:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DA4368911\n\tfor ;\n\tSun, 16 May 2021 16:06:41 +0200 (CEST)","by mail-lj1-x22c.google.com with SMTP id p12so4102183ljg.1\n\tfor ;\n\tSun, 16 May 2021 07:06:41 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tc10sm1768549ljr.85.2021.05.16.07.06.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 16 May 2021 07:06:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"rzD4itYJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=udutVOYugiSqwk6X033NQru00UxQRmud2LnyT/4zh/Y=;\n\tb=rzD4itYJDWzPe4vNH6o3HY+4bajdna9WB0G8KH4tYBghsh+7cUr+lpLQ5Aoseeb5ea\n\tvY993q1SF2hVTKTqiP8f7viWy3TeuCCAWMHXib3XwEKPLgmhDTvwUmuix5Nq+ZyGbGyj\n\tdpCLeOclN6EtYe/ddLabRpJBJV1sKM4UEf4UQPTDL+9V8ztSugLGatPqtmyps//F5VTq\n\tpc4WwcKCKpfizgtCfHwSgVO3eNspy4nAqhWzZb7zYt/wCndtIF39hg22FEV4NSRTey77\n\tJTIn9GFc1b1WyrfWl8C9apmi/Qnp1e95saKhyTgvnycois+VPw5emotszlvYZE6T3eND\n\tBnJA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=udutVOYugiSqwk6X033NQru00UxQRmud2LnyT/4zh/Y=;\n\tb=Zj+KdCDUCqktEk5Ug1gPaPr1m0H68w/XmHdcEEmRaLjC/9gt2eMg8jQ58wj0HyY1r/\n\tuYksCbnAxBfql8TLBf4Zx/3KJRm072zH9YdDHZwUM3nR99hXbxZdUYTVwV6DAndCvyHh\n\t5VOgJ2gEJBMaF8ACjC8e6v7w4uGjRPSWbZXo+rnIaqGJ9BPmFMBTb3blElgl1a96Gfqf\n\tgtVLICVVzxer9XDJvOnu1XQ6zp54o8kuGWDMLpYsZf8/ZRj11C4BWchaqnEIiJEgGbDO\n\ttJG3EOwjmcoGntEkimmv4j0EuG5Se/mTeuj1Qqm6/VXJryILs+NTQMneNOGsOx53ACTb\n\t34kA==","X-Gm-Message-State":"AOAM532ZVo98poF3nDGhygbP5EGgCYs2z2NH9AbAB9xkFuaeqN+ZRFRK\n\tLYNyRz7bO815Byzq9X7AUeSgwA==","X-Google-Smtp-Source":"ABdhPJw/uGxCMdpkOjf6jHIXqSIEwihPjaoMwqbvw/jUVFEUlh98j4zkWxdyU0371O021IJxSpTBog==","X-Received":"by 2002:a05:651c:1182:: with SMTP id\n\tw2mr45417702ljo.108.1621174000811; \n\tSun, 16 May 2021 07:06:40 -0700 (PDT)","Date":"Sun, 16 May 2021 16:06:39 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"<20210514131652.345486-1-nfraprado@collabora.com>\n\t<20210514131652.345486-5-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210514131652.345486-5-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] lc-compliance: Add list and\n\tfilter parameters","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17050,"web_url":"https://patchwork.libcamera.org/comment/17050/","msgid":"","date":"2021-05-20T17:46:21","subject":"Re: [libcamera-devel] [PATCH v3 4/4] lc-compliance: Add list and\n\tfilter parameters","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Niklas,\n\nthanks for the feedback.\n\nEm 2021-05-16 11:06, Niklas Söderlund escreveu:\n\n> Hi Nícolas,\n>\n> Thanks for your work.\n>\n> On 2021-05-14 10:16:52 -0300, Nícolas F. R. A. Prado wrote:\n> > Add a --list parameter that lists all current tests (by mapping to\n> > googletest's --gtest_list_tests).\n> > \n> > Add a --filter 'filterString' parameter that filters the tests to run\n> > (by mapping to googletest's --gtest_filter).\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > src/lc-compliance/main.cpp | 68 ++++++++++++++++++++++++++++++++++++--\n> > 1 file changed, 66 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index d5ff93f514df..bebf1c51f64b 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -20,6 +20,8 @@ using namespace libcamera;\n> > \n> > enum {\n> > \tOptCamera = 'c',\n> > +\tOptList = 'l',\n> > +\tOptFilter = 'f',\n> > \tOptHelp = 'h',\n> > };\n> > \n> > @@ -54,16 +56,26 @@ public:\n> > \t~Harness();\n> > \n> > \tint init();\n> > +\tint buildGtestParameters(char *arg0);\n> > +\tint gtestArgc() { return gtestArgc_; };\n> > +\tchar **gtestArgv() { return gtestArgv_; };\n>\n> This feels a bit like a layer violation.\n>\n> Do you think it would make sens to instead move the gtest data\n> structures inside the Harness class? Or perhaps the Harness class is not\n> as useful now that it likely won't grow to support all the test\n> infrastructure provided by gtest and should instead be removed and what\n> is left of it moved to main() and such helpers?\n\nYou mean moving \n\n\t::testing::InitGoogleTest(>estArgc, harness.gtestArgv());\n\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n\treturn RUN_ALL_TESTS();\n\ninside the Harness function and removing those getters, right? Yeah, seems like\na good idea.\n\nThanks,\nNícolas\n\n>\n> > \n> > private:\n> > \tvoid listCameras();\n> > \n> > \tOptionsParser::Options options_;\n> > \tstd::unique_ptr cm_;\n> > +\n> > +\tconst std::map gtestFlag_ = {{\"list\", \"--gtest_list_tests\"},\n> > +\t\t\t\t\t\t\t {\"filter\", \"--gtest_filter\"}};\n> > +\n> > +\tint gtestArgc_;\n> > +\tchar **gtestArgv_;\n> > +\tstd::string gtestFilterParam_;\n> > };\n> > \n> > Harness::Harness(const OptionsParser::Options &options)\n> > -\t: options_(options)\n> > +\t: options_(options), gtestArgv_(nullptr)\n> > {\n> > \tcm_ = std::make_unique();\n> > }\n> > @@ -73,6 +85,8 @@ Harness::~Harness()\n> > \tEnvironment::instance()->destroy();\n> > \n> > \tcm_->stop();\n> > +\n> > +\tfree(gtestArgv_);\n> > }\n> > \n> > int Harness::init()\n> > @@ -86,6 +100,9 @@ int Harness::init()\n> > \t\treturn ret;\n> > \t}\n> > \n> > +\tif (options_.isSet(OptList))\n> > +\t\treturn 0;\n> > +\n> > \tif (!options_.isSet(OptCamera)) {\n> > \t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> > \t\tlistCameras();\n> > @@ -118,12 +135,54 @@ void Harness::listCameras()\n> > \t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> > }\n> > \n> > +int Harness::buildGtestParameters(char *arg0)\n> > +{\n> > +\tint argc = 0;\n> > +\n> > +\t/*\n> > +\t * +2 to have space for both the 0th argument that is needed but not\n> > +\t * used and the null at the end.\n> > +\t */\n> > +\tgtestArgv_ = (char**) malloc((gtestFlag_.size() + 2) * sizeof(char*));\n> > +\tif (!gtestArgv_)\n> > +\t\treturn -ENOMEM;\n> > +\n> > +\tgtestArgv_[argc] = arg0;\n> > +\targc++;\n> > +\n> > +\tif (options_.isSet(OptList)) {\n> > +\t\tgtestArgv_[argc] = const_cast(gtestFlag_.at(\"list\").c_str());\n> > +\t\targc++;\n> > +\t}\n> > +\n> > +\tif (options_.isSet(OptFilter)) {\n> > +\t\t/*\n> > +\t\t * The filter flag needs to be passed as a single parameter, in\n> > +\t\t * the format --gtest_filter=filterStr\n> > +\t\t */\n> > +\t\tconst std::string &filter = options_[OptFilter];\n> > +\t\tgtestFilterParam_ = gtestFlag_.at(\"filter\") + \"=\" + filter;\n> > +\n> > +\t\tgtestArgv_[argc] = const_cast(gtestFilterParam_.c_str());\n> > +\t\targc++;\n> > +\t}\n> > +\n> > +\tgtestArgv_[argc] = 0;\n> > +\tgtestArgc_ = argc;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> > {\n> > \tOptionsParser parser;\n> > \tparser.addOption(OptCamera, OptionString,\n> > \t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n> > \t\t\t ArgumentRequired, \"camera\");\n> > +\tparser.addOption(OptList, OptionNone, \"List all tests and exit\", \"list\");\n> > +\tparser.addOption(OptFilter, OptionString,\n> > +\t\t\t \"Specify which tests to run\", \"filter\",\n> > +\t\t\t ArgumentRequired, \"filter\");\n> > \tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n> > \t\t\t \"help\");\n> > \n> > @@ -170,7 +229,12 @@ int main(int argc, char **argv)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\t::testing::InitGoogleTest(&argc, argv);\n> > +\tret = harness.buildGtestParameters(argv[0]);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\tint gtestArgc = harness.gtestArgc();\n> > +\n> > +\t::testing::InitGoogleTest(>estArgc, harness.gtestArgv());\n> > \ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> > \treturn RUN_ALL_TESTS();\n> > }\n> > -- \n> > 2.31.1\n> > \n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0F0D1C31FB\n\tfor ;\n\tThu, 20 May 2021 17:47:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7259A6891F;\n\tThu, 20 May 2021 19:47:02 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECD59602B2\n\tfor ;\n\tThu, 20 May 2021 19:47:00 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:d269:289c:2421:e513])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 3A2F81F43DA7;\n\tThu, 20 May 2021 18:46:58 +0100 (BST)"],"Mime-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=UTF-8","Date":"Thu, 20 May 2021 14:46:21 -0300","Message-Id":"","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","References":"<20210514131652.345486-1-nfraprado@collabora.com>\n\t<20210514131652.345486-5-nfraprado@collabora.com>\n\t","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/4] lc-compliance: Add list and\n\tfilter parameters","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]