[{"id":299,"web_url":"https://patchwork.libcamera.org/comment/299/","msgid":"<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>","date":"2019-01-14T10:46:04","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:\n> Provide Class object to return the test status, and perform any result reporting.\n>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  test/libtest/meson.build     |  1 +\n>  test/libtest/test.h          |  2 ++\n>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n>  4 files changed, 100 insertions(+)\n>  create mode 100644 test/libtest/test_status.cpp\n>  create mode 100644 test/libtest/test_status.h\n>\n> diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n> index e0893b70c3d4..a9e67d8d7e6c 100644\n> --- a/test/libtest/meson.build\n> +++ b/test/libtest/meson.build\n> @@ -1,5 +1,6 @@\n>  libtest_sources = files([\n>      'test.cpp',\n> +    'test_status.cpp',\n>  ])\n>\n>  libtest = static_library('libtest', libtest_sources)\n> diff --git a/test/libtest/test.h b/test/libtest/test.h\n> index ec08bf97c03d..d816cf15aaf0 100644\n> --- a/test/libtest/test.h\n> +++ b/test/libtest/test.h\n> @@ -9,6 +9,8 @@\n>\n>  #include <sstream>\n>\n> +#include \"test_status.h\"\n> +\n>  enum TestStatus {\n>  \tTestPass = 0,\n>  \tTestFail = -1,\n\nWhen you will replace this, please move it to test_status.h\n\n> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n> new file mode 100644\n> index 000000000000..c0daef866740\n> --- /dev/null\n> +++ b/test/libtest/test_status.cpp\n> @@ -0,0 +1,42 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * test_status.cpp - libcamera test result management\n> + */\n> +\n> +#include \"test.h\"\n> +\n> +static unsigned int test_number = 0;\n> +\n> +TestStatusBase::TestStatusBase()\n> +\t: value(-99)\n\n-99 ?\n\n> +{\n> +\ttest_number++;\n> +};\n> +\n> +TestStatusBase::~TestStatusBase()\n> +{\n> +\tstd::cout << prefix << test_number << \" \" << message << std::endl;\n\ncout or cerr? This has been asked already bu never finalized\n\n> +};\n> +\n> +TestStatusPass::TestStatusPass(const std::string &m)\n> +{\n> +\tvalue = ValuePass;\n> +\tprefix = \"ok \";\n> +\tmessage = m;\n> +};\n> +\n> +TestStatusFail::TestStatusFail(const std::string &m)\n> +{\n> +\tvalue = ValueFail;\n> +\tprefix = \"not ok \";\n> +\tmessage = m;\n> +};\n> +\n> +TestStatusSkip::TestStatusSkip(const std::string &m)\n> +{\n> +\tvalue = ValueSkip;\n> +\tprefix = \"skip \";\n> +\tmessage = m;\n> +};\n\nUse the base constructor, as noted below in a comment to the header\nfile\n\n> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n> new file mode 100644\n> index 000000000000..2e4713ad3f6d\n> --- /dev/null\n> +++ b/test/libtest/test_status.h\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * test_status.h - libcamera test status class\n\nMake this and the .cpp one the same\n\n> + */\n> +#ifndef __TEST_TEST_STATUS_H__\n> +#define __TEST_TEST_STATUS_H__\n> +\n> +#include <iostream>\n\nYou can move this to the cpp file\n\n> +#include <string>\n> +\n> +class TestStatusBase\n> +{\n> +public:\n> +\tTestStatusBase();\n> +\tTestStatusBase(const std::string &m);\n\nTestStatusBase() is not used and shall be deleted\nTestStatusBase(const std::string m) should be protected as this base\nclass shall not be instantiated directly\n\n> +\t~TestStatusBase();\n> +\n> +\toperator int() { return value; };\n> +\n> +\tenum ReturnValues {\n> +\t\tValuePass = 0,\n> +\t\tValueFail = -1,\n> +\t\tValueSkip = 77,\n> +\t};\n\nThis and TestStatus can be unified\n\n> +\n> +protected:\n> +\tint value;\n> +\tstd::string prefix;\n> +\tstd::string message;\n\nclass member end with _\n> +};\n> +\n> +class TestStatusPass : public TestStatusBase\n\nAll of these shall use their base constructor\n> +{\n> +public:\n> +\tTestStatusPass(const std::string &m);\n> +};\n> +\n> +class TestStatusFail : public TestStatusBase\n> +{\n> +public:\n> +\tTestStatusFail(const std::string &m);\n> +};\n> +\n> +class TestStatusSkip : public TestStatusBase\n> +{\n> +public:\n> +\tTestStatusSkip(const std::string &m);\n> +};\n> +\n> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n> +\n> +#endif /* __TEST_TEST_STATUS_H__ */\n\nHere it is a patch on top that addresses most of my comments and that\nproduces this output when run through the test_status_test\n\n/home/jmondi/project/libcamera/libcamera.git/build/test/test_status\n--- stdout ---\nTest: 1: ok - [Verify TestStatusPass]\nTest: 2: not ok - [Verify TestStatusFail]\nTest: 3: skip - [Verify TestStatusSkip]\nTest: 4: ok - [Good is return check]\nTest: 5: ok - [Good isn't return check]\nTest: 6: not ok - [Bad Is Check]\nTest: 7: not ok - [Bad Isn't check]\nTest: 8: ok - TestStatus validations\n-------\n\ndiff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\nindex c0daef8..be84556 100644\n--- a/test/libtest/test_status.cpp\n+++ b/test/libtest/test_status.cpp\n@@ -5,38 +5,37 @@\n  * test_status.cpp - libcamera test result management\n  */\n\n+#include <iostream>\n+#include <string>\n+\n #include \"test.h\"\n\n static unsigned int test_number = 0;\n\n-TestStatusBase::TestStatusBase()\n-       : value(-99)\n+TestStatusBase::TestStatusBase(enum ReturnValues result,\n+                              const std::string &prefix,\n+                              const std::string &message)\n+       : result_(result), prefix_(prefix), message_(message)\n {\n        test_number++;\n };\n\n TestStatusBase::~TestStatusBase()\n {\n-       std::cout << prefix << test_number << \" \" << message << std::endl;\n+       std::cout << \"Test: \" << test_number << \": \" << prefix_ << \" - \"\n+                 << message_ << std::endl;\n\n };\n\n-TestStatusPass::TestStatusPass(const std::string &m)\n+TestStatusPass::TestStatusPass(const std::string &m) :\n+       TestStatusBase(ValuePass, \"ok\", m)\n {\n-       value = ValuePass;\n-       prefix = \"ok \";\n-       message = m;\n };\n\n-TestStatusFail::TestStatusFail(const std::string &m)\n+TestStatusFail::TestStatusFail(const std::string &m) :\n+       TestStatusBase(ValueFail, \"not ok\", m)\n {\n-       value = ValueFail;\n-       prefix = \"not ok \";\n-       message = m;\n };\n\n-TestStatusSkip::TestStatusSkip(const std::string &m)\n+TestStatusSkip::TestStatusSkip(const std::string &m) :\n+       TestStatusBase(ValueSkip, \"skip\", m)\n {\n-       value = ValueSkip;\n-       prefix = \"skip \";\n-       message = m;\n };\ndiff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\nindex 2e4713a..7f3bb26 100644\n--- a/test/libtest/test_status.h\n+++ b/test/libtest/test_status.h\n@@ -7,17 +7,14 @@\n #ifndef __TEST_TEST_STATUS_H__\n #define __TEST_TEST_STATUS_H__\n\n-#include <iostream>\n #include <string>\n\n class TestStatusBase\n {\n public:\n-       TestStatusBase();\n-       TestStatusBase(const std::string &m);\n        ~TestStatusBase();\n\n-       operator int() { return value; };\n+       operator int() { return result_; };\n\n        enum ReturnValues {\n                ValuePass = 0,\n@@ -26,9 +23,12 @@ public:\n        };\n\n protected:\n-       int value;\n-       std::string prefix;\n public:\n-       TestStatusBase();\n-       TestStatusBase(const std::string &m);\n        ~TestStatusBase();\n\n-       operator int() { return value; };\n+       operator int() { return result_; };\n\n        enum ReturnValues {\n                ValuePass = 0,\n@@ -26,9 +23,12 @@ public:\n        };\n\n protected:\n-       int value;\n-       std::string prefix;\n-       std::string message;\n+       TestStatusBase() = delete;\n+       TestStatusBase(enum ReturnValues result, const std::string &prefix,\n+                      const std::string &m);\n+       enum ReturnValues result_;\n+       std::string prefix_;\n+       std::string message_;\n };\n\n class TestStatusPass : public TestStatusBase\n\n\n> --\n> 2.17.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91F4B60C71\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 11:45:55 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 0F3AD1C0004;\n\tMon, 14 Jan 2019 10:45:54 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 14 Jan 2019 11:46:04 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114095417.16473-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"a3giapitacl7q3ym\"","Content-Disposition":"inline","In-Reply-To":"<20190114095417.16473-2-kieran.bingham@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 10:45:55 -0000"}},{"id":301,"web_url":"https://patchwork.libcamera.org/comment/301/","msgid":"<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","date":"2019-01-14T13:24:49","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks - Shall I take your detailed review comments as a good indicator\nthat you like this direction?\n\nI'm sorry it was not so polished, (hence the RFC) - I wanted to gauge\nfeel on if this is a good way forwards as much as showing the\nimplementation.\n\nThat said - you've fixed up the initialiser for the three classes for me :)\n\nSo now I can use:\n\nTestPass::TestPass(const std::string &m) :\n     TestStatus(ValuePass, \"ok\", m) {}\n\n\n\nOn 14/01/2019 10:46, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:\n>> Provide Class object to return the test status, and perform any result reporting.\n>>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>> ---\n>>  test/libtest/meson.build     |  1 +\n>>  test/libtest/test.h          |  2 ++\n>>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n>>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n>>  4 files changed, 100 insertions(+)\n>>  create mode 100644 test/libtest/test_status.cpp\n>>  create mode 100644 test/libtest/test_status.h\n>>\n>> diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n>> index e0893b70c3d4..a9e67d8d7e6c 100644\n>> --- a/test/libtest/meson.build\n>> +++ b/test/libtest/meson.build\n>> @@ -1,5 +1,6 @@\n>>  libtest_sources = files([\n>>      'test.cpp',\n>> +    'test_status.cpp',\n>>  ])\n>>\n>>  libtest = static_library('libtest', libtest_sources)\n>> diff --git a/test/libtest/test.h b/test/libtest/test.h\n>> index ec08bf97c03d..d816cf15aaf0 100644\n>> --- a/test/libtest/test.h\n>> +++ b/test/libtest/test.h\n>> @@ -9,6 +9,8 @@\n>>\n>>  #include <sstream>\n>>\n>> +#include \"test_status.h\"\n>> +\n>>  enum TestStatus {\n>>  \tTestPass = 0,\n>>  \tTestFail = -1,\n> \n> When you will replace this, please move it to test_status.h\n\nThey already are - they are there as ValuePass. I aim to rename the\nclass TestStatusPass() to class TestPass() and thus this enum TestStatus\nshall then be removed completely.\n\nI've kept it during this RFC to allow both to live side by side for\ntrying out the new style - and providing comments :)\n\n> \n>> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n>> new file mode 100644\n>> index 000000000000..c0daef866740\n>> --- /dev/null\n>> +++ b/test/libtest/test_status.cpp\n>> @@ -0,0 +1,42 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * test_status.cpp - libcamera test result management\n>> + */\n>> +\n>> +#include \"test.h\"\n>> +\n>> +static unsigned int test_number = 0;\n>> +\n>> +TestStatusBase::TestStatusBase()\n>> +\t: value(-99)\n> \n> -99 ?\n\nI needed a random non-defined number. TestStatusBase shouldn't be\ninstantiated.\n\n\n>> +{\n>> +\ttest_number++;\n>> +};\n>> +\n>> +TestStatusBase::~TestStatusBase()\n>> +{\n>> +\tstd::cout << prefix << test_number << \" \" << message << std::endl;\n> \n> cout or cerr? This has been asked already bu never finalized\n\nWell the best part about this series is that there will be only one\nplace to change when we decide :)\n\ncout will be fine for the moment I think.\n\n\n> \n>> +};\n>> +\n>> +TestStatusPass::TestStatusPass(const std::string &m)\n>> +{\n>> +\tvalue = ValuePass;\n>> +\tprefix = \"ok \";\n>> +\tmessage = m;\n>> +};\n>> +\n>> +TestStatusFail::TestStatusFail(const std::string &m)\n>> +{\n>> +\tvalue = ValueFail;\n>> +\tprefix = \"not ok \";\n>> +\tmessage = m;\n>> +};\n>> +\n>> +TestStatusSkip::TestStatusSkip(const std::string &m)\n>> +{\n>> +\tvalue = ValueSkip;\n>> +\tprefix = \"skip \";\n>> +\tmessage = m;\n>> +};\n> \n> Use the base constructor, as noted below in a comment to the header\n> file\n\nThank you for the patch -  I tried to do this through the initialiser\nlist - and couldn't get it to work correctly.\n\nNow I see I was missing the wrapping for the TestBase() - thanks I'll\nupdate.\n\n\n\n> \n>> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n>> new file mode 100644\n>> index 000000000000..2e4713ad3f6d\n>> --- /dev/null\n>> +++ b/test/libtest/test_status.h\n>> @@ -0,0 +1,55 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * test_status.h - libcamera test status class\n> \n> Make this and the .cpp one the same\n> \n>> + */\n>> +#ifndef __TEST_TEST_STATUS_H__\n>> +#define __TEST_TEST_STATUS_H__\n>> +\n>> +#include <iostream>\n> \n> You can move this to the cpp file\n> \n>> +#include <string>\n>> +\n>> +class TestStatusBase\n>> +{\n>> +public:\n>> +\tTestStatusBase();\n>> +\tTestStatusBase(const std::string &m);\n> \n> TestStatusBase() is not used and shall be deleted\n> TestStatusBase(const std::string m) should be protected as this base\n> class shall not be instantiated directly\n\nCorrect :)\n\n\n> \n>> +\t~TestStatusBase();\n>> +\n>> +\toperator int() { return value; };\n>> +\n>> +\tenum ReturnValues {\n>> +\t\tValuePass = 0,\n>> +\t\tValueFail = -1,\n>> +\t\tValueSkip = 77,\n>> +\t};\n> \n> This and TestStatus can be unified\n\nNot quite - I want to use the TestPass TestSkip, TestFail namings for\nthe actual object names.\n\n\n> \n>> +\n>> +protected:\n>> +\tint value;\n>> +\tstd::string prefix;\n>> +\tstd::string message;\n> \n> class member end with _\n>> +};\n>> +\n>> +class TestStatusPass : public TestStatusBase\n> \n> All of these shall use their base constructor\n>> +{\n>> +public:\n>> +\tTestStatusPass(const std::string &m);\n>> +};\n>> +\n>> +class TestStatusFail : public TestStatusBase\n>> +{\n>> +public:\n>> +\tTestStatusFail(const std::string &m);\n>> +};\n>> +\n>> +class TestStatusSkip : public TestStatusBase\n>> +{\n>> +public:\n>> +\tTestStatusSkip(const std::string &m);\n>> +};\n>> +\n>> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n>> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n>> +\n>> +#endif /* __TEST_TEST_STATUS_H__ */\n> \n> Here it is a patch on top that addresses most of my comments and that\n> produces this output when run through the test_status_test\n> \n> /home/jmondi/project/libcamera/libcamera.git/build/test/test_status\n> --- stdout ---\n> Test: 1: ok - [Verify TestStatusPass]\n> Test: 2: not ok - [Verify TestStatusFail]\n> Test: 3: skip - [Verify TestStatusSkip]\n> Test: 4: ok - [Good is return check]\n> Test: 5: ok - [Good isn't return check]\n> Test: 6: not ok - [Bad Is Check]\n> Test: 7: not ok - [Bad Isn't check]\n> Test: 8: ok - TestStatus validations\n> -------\n> \n> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n> index c0daef8..be84556 100644\n> --- a/test/libtest/test_status.cpp\n> +++ b/test/libtest/test_status.cpp\n> @@ -5,38 +5,37 @@\n>   * test_status.cpp - libcamera test result management\n>   */\n> \n> +#include <iostream>\n> +#include <string>\n> +\n>  #include \"test.h\"\n> \n>  static unsigned int test_number = 0;\n> \n> -TestStatusBase::TestStatusBase()\n> -       : value(-99)\n> +TestStatusBase::TestStatusBase(enum ReturnValues result,\n> +                              const std::string &prefix,\n> +                              const std::string &message)\n> +       : result_(result), prefix_(prefix), message_(message)\n>  {\n>         test_number++;\n>  };\n> \n>  TestStatusBase::~TestStatusBase()\n>  {\n> -       std::cout << prefix << test_number << \" \" << message << std::endl;\n> +       std::cout << \"Test: \" << test_number << \": \" << prefix_ << \" - \"\n> +                 << message_ << std::endl;\n> \n>  };\n> \n> -TestStatusPass::TestStatusPass(const std::string &m)\n> +TestStatusPass::TestStatusPass(const std::string &m) :\n> +       TestStatusBase(ValuePass, \"ok\", m)\n>  {\n> -       value = ValuePass;\n> -       prefix = \"ok \";\n> -       message = m;\n>  };\n> \n> -TestStatusFail::TestStatusFail(const std::string &m)\n> +TestStatusFail::TestStatusFail(const std::string &m) :\n> +       TestStatusBase(ValueFail, \"not ok\", m)\n>  {\n> -       value = ValueFail;\n> -       prefix = \"not ok \";\n> -       message = m;\n>  };\n> \n> -TestStatusSkip::TestStatusSkip(const std::string &m)\n> +TestStatusSkip::TestStatusSkip(const std::string &m) :\n> +       TestStatusBase(ValueSkip, \"skip\", m)\n>  {\n> -       value = ValueSkip;\n> -       prefix = \"skip \";\n> -       message = m;\n>  };\n> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n> index 2e4713a..7f3bb26 100644\n> --- a/test/libtest/test_status.h\n> +++ b/test/libtest/test_status.h\n> @@ -7,17 +7,14 @@\n>  #ifndef __TEST_TEST_STATUS_H__\n>  #define __TEST_TEST_STATUS_H__\n> \n> -#include <iostream>\n>  #include <string>\n> \n>  class TestStatusBase\n>  {\n>  public:\n> -       TestStatusBase();\n> -       TestStatusBase(const std::string &m);\n>         ~TestStatusBase();\n> \n> -       operator int() { return value; };\n> +       operator int() { return result_; };\n> \n>         enum ReturnValues {\n>                 ValuePass = 0,\n> @@ -26,9 +23,12 @@ public:\n>         };\n> \n>  protected:\n> -       int value;\n> -       std::string prefix;\n>  public:\n> -       TestStatusBase();\n> -       TestStatusBase(const std::string &m);\n>         ~TestStatusBase();\n> \n> -       operator int() { return value; };\n> +       operator int() { return result_; };\n> \n>         enum ReturnValues {\n>                 ValuePass = 0,\n> @@ -26,9 +23,12 @@ public:\n>         };\n> \n>  protected:\n> -       int value;\n> -       std::string prefix;\n> -       std::string message;\n> +       TestStatusBase() = delete;\n> +       TestStatusBase(enum ReturnValues result, const std::string &prefix,\n> +                      const std::string &m);\n\n> +       enum ReturnValues result_;\n\nAh - yes - correct value type is a good call too :)\n\n> +       std::string prefix_;\n> +       std::string message_;\n>  };\n> \n>  class TestStatusPass : public TestStatusBase\n> \n> \n>> --\n>> 2.17.1\n>>\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CCF660C79\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 14:24:52 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC37C530;\n\tMon, 14 Jan 2019 14:24:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547472291;\n\tbh=GwCpQhkBc9Qxbxm6HSAWs1PT5p5wOTSQ+8LTmniQm8I=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=IH3FU88rmFp7HYxG8Cy1f4EvkNCDUq1QRg8Uq7wx8fw+y6mu85cqCTtqXmpRc0Bjh\n\tuv2T2fBJ+JJw7BLMZjtvMTZRJxMJnTbJrFhSYFt9uqtzgA8c6LbKPsYaUGBsEnIHW6\n\tKxdtFQqAfs6SmW4it7obor1fMIKmePXlNgLwPJhM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114095417.16473-2-kieran.bingham@ideasonboard.com>\n\t<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","Date":"Mon, 14 Jan 2019 13:24:49 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 13:24:52 -0000"}},{"id":302,"web_url":"https://patchwork.libcamera.org/comment/302/","msgid":"<20190114133527.sguw6vpm3b7bkm6k@uno.localdomain>","date":"2019-01-14T13:35:27","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Jan 14, 2019 at 01:24:49PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> Thanks - Shall I take your detailed review comments as a good indicator\n> that you like this direction?\n>\n\nYes I do :)\n\n> I'm sorry it was not so polished, (hence the RFC) - I wanted to gauge\n> feel on if this is a good way forwards as much as showing the\n> implementation.\n>\n> That said - you've fixed up the initialiser for the three classes for me :)\n>\n> So now I can use:\n>\n> TestPass::TestPass(const std::string &m) :\n>      TestStatus(ValuePass, \"ok\", m) {}\n\ngreat, but please not my below patches do not respect the \"TAP\" output\nformat, I just found out what was about after sending this.\n\nHow do we plan to use the TAP formatted output?\n\nThanks\n   j\n\n>\n>\n>\n> On 14/01/2019 10:46, Jacopo Mondi wrote:\n> > Hi Kieran,\n> >\n> > On Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:\n> >> Provide Class object to return the test status, and perform any result reporting.\n> >>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >>  test/libtest/meson.build     |  1 +\n> >>  test/libtest/test.h          |  2 ++\n> >>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n> >>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n> >>  4 files changed, 100 insertions(+)\n> >>  create mode 100644 test/libtest/test_status.cpp\n> >>  create mode 100644 test/libtest/test_status.h\n> >>\n> >> diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n> >> index e0893b70c3d4..a9e67d8d7e6c 100644\n> >> --- a/test/libtest/meson.build\n> >> +++ b/test/libtest/meson.build\n> >> @@ -1,5 +1,6 @@\n> >>  libtest_sources = files([\n> >>      'test.cpp',\n> >> +    'test_status.cpp',\n> >>  ])\n> >>\n> >>  libtest = static_library('libtest', libtest_sources)\n> >> diff --git a/test/libtest/test.h b/test/libtest/test.h\n> >> index ec08bf97c03d..d816cf15aaf0 100644\n> >> --- a/test/libtest/test.h\n> >> +++ b/test/libtest/test.h\n> >> @@ -9,6 +9,8 @@\n> >>\n> >>  #include <sstream>\n> >>\n> >> +#include \"test_status.h\"\n> >> +\n> >>  enum TestStatus {\n> >>  \tTestPass = 0,\n> >>  \tTestFail = -1,\n> >\n> > When you will replace this, please move it to test_status.h\n>\n> They already are - they are there as ValuePass. I aim to rename the\n> class TestStatusPass() to class TestPass() and thus this enum TestStatus\n> shall then be removed completely.\n>\n> I've kept it during this RFC to allow both to live side by side for\n> trying out the new style - and providing comments :)\n>\n> >\n> >> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n> >> new file mode 100644\n> >> index 000000000000..c0daef866740\n> >> --- /dev/null\n> >> +++ b/test/libtest/test_status.cpp\n> >> @@ -0,0 +1,42 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * test_status.cpp - libcamera test result management\n> >> + */\n> >> +\n> >> +#include \"test.h\"\n> >> +\n> >> +static unsigned int test_number = 0;\n> >> +\n> >> +TestStatusBase::TestStatusBase()\n> >> +\t: value(-99)\n> >\n> > -99 ?\n>\n> I needed a random non-defined number. TestStatusBase shouldn't be\n> instantiated.\n>\n>\n> >> +{\n> >> +\ttest_number++;\n> >> +};\n> >> +\n> >> +TestStatusBase::~TestStatusBase()\n> >> +{\n> >> +\tstd::cout << prefix << test_number << \" \" << message << std::endl;\n> >\n> > cout or cerr? This has been asked already bu never finalized\n>\n> Well the best part about this series is that there will be only one\n> place to change when we decide :)\n>\n> cout will be fine for the moment I think.\n>\n>\n> >\n> >> +};\n> >> +\n> >> +TestStatusPass::TestStatusPass(const std::string &m)\n> >> +{\n> >> +\tvalue = ValuePass;\n> >> +\tprefix = \"ok \";\n> >> +\tmessage = m;\n> >> +};\n> >> +\n> >> +TestStatusFail::TestStatusFail(const std::string &m)\n> >> +{\n> >> +\tvalue = ValueFail;\n> >> +\tprefix = \"not ok \";\n> >> +\tmessage = m;\n> >> +};\n> >> +\n> >> +TestStatusSkip::TestStatusSkip(const std::string &m)\n> >> +{\n> >> +\tvalue = ValueSkip;\n> >> +\tprefix = \"skip \";\n> >> +\tmessage = m;\n> >> +};\n> >\n> > Use the base constructor, as noted below in a comment to the header\n> > file\n>\n> Thank you for the patch -  I tried to do this through the initialiser\n> list - and couldn't get it to work correctly.\n>\n> Now I see I was missing the wrapping for the TestBase() - thanks I'll\n> update.\n>\n>\n>\n> >\n> >> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n> >> new file mode 100644\n> >> index 000000000000..2e4713ad3f6d\n> >> --- /dev/null\n> >> +++ b/test/libtest/test_status.h\n> >> @@ -0,0 +1,55 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * test_status.h - libcamera test status class\n> >\n> > Make this and the .cpp one the same\n> >\n> >> + */\n> >> +#ifndef __TEST_TEST_STATUS_H__\n> >> +#define __TEST_TEST_STATUS_H__\n> >> +\n> >> +#include <iostream>\n> >\n> > You can move this to the cpp file\n> >\n> >> +#include <string>\n> >> +\n> >> +class TestStatusBase\n> >> +{\n> >> +public:\n> >> +\tTestStatusBase();\n> >> +\tTestStatusBase(const std::string &m);\n> >\n> > TestStatusBase() is not used and shall be deleted\n> > TestStatusBase(const std::string m) should be protected as this base\n> > class shall not be instantiated directly\n>\n> Correct :)\n>\n>\n> >\n> >> +\t~TestStatusBase();\n> >> +\n> >> +\toperator int() { return value; };\n> >> +\n> >> +\tenum ReturnValues {\n> >> +\t\tValuePass = 0,\n> >> +\t\tValueFail = -1,\n> >> +\t\tValueSkip = 77,\n> >> +\t};\n> >\n> > This and TestStatus can be unified\n>\n> Not quite - I want to use the TestPass TestSkip, TestFail namings for\n> the actual object names.\n>\n>\n> >\n> >> +\n> >> +protected:\n> >> +\tint value;\n> >> +\tstd::string prefix;\n> >> +\tstd::string message;\n> >\n> > class member end with _\n> >> +};\n> >> +\n> >> +class TestStatusPass : public TestStatusBase\n> >\n> > All of these shall use their base constructor\n> >> +{\n> >> +public:\n> >> +\tTestStatusPass(const std::string &m);\n> >> +};\n> >> +\n> >> +class TestStatusFail : public TestStatusBase\n> >> +{\n> >> +public:\n> >> +\tTestStatusFail(const std::string &m);\n> >> +};\n> >> +\n> >> +class TestStatusSkip : public TestStatusBase\n> >> +{\n> >> +public:\n> >> +\tTestStatusSkip(const std::string &m);\n> >> +};\n> >> +\n> >> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n> >> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n> >> +\n> >> +#endif /* __TEST_TEST_STATUS_H__ */\n> >\n> > Here it is a patch on top that addresses most of my comments and that\n> > produces this output when run through the test_status_test\n> >\n> > /home/jmondi/project/libcamera/libcamera.git/build/test/test_status\n> > --- stdout ---\n> > Test: 1: ok - [Verify TestStatusPass]\n> > Test: 2: not ok - [Verify TestStatusFail]\n> > Test: 3: skip - [Verify TestStatusSkip]\n> > Test: 4: ok - [Good is return check]\n> > Test: 5: ok - [Good isn't return check]\n> > Test: 6: not ok - [Bad Is Check]\n> > Test: 7: not ok - [Bad Isn't check]\n> > Test: 8: ok - TestStatus validations\n> > -------\n> >\n> > diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n> > index c0daef8..be84556 100644\n> > --- a/test/libtest/test_status.cpp\n> > +++ b/test/libtest/test_status.cpp\n> > @@ -5,38 +5,37 @@\n> >   * test_status.cpp - libcamera test result management\n> >   */\n> >\n> > +#include <iostream>\n> > +#include <string>\n> > +\n> >  #include \"test.h\"\n> >\n> >  static unsigned int test_number = 0;\n> >\n> > -TestStatusBase::TestStatusBase()\n> > -       : value(-99)\n> > +TestStatusBase::TestStatusBase(enum ReturnValues result,\n> > +                              const std::string &prefix,\n> > +                              const std::string &message)\n> > +       : result_(result), prefix_(prefix), message_(message)\n> >  {\n> >         test_number++;\n> >  };\n> >\n> >  TestStatusBase::~TestStatusBase()\n> >  {\n> > -       std::cout << prefix << test_number << \" \" << message << std::endl;\n> > +       std::cout << \"Test: \" << test_number << \": \" << prefix_ << \" - \"\n> > +                 << message_ << std::endl;\n> >\n> >  };\n> >\n> > -TestStatusPass::TestStatusPass(const std::string &m)\n> > +TestStatusPass::TestStatusPass(const std::string &m) :\n> > +       TestStatusBase(ValuePass, \"ok\", m)\n> >  {\n> > -       value = ValuePass;\n> > -       prefix = \"ok \";\n> > -       message = m;\n> >  };\n> >\n> > -TestStatusFail::TestStatusFail(const std::string &m)\n> > +TestStatusFail::TestStatusFail(const std::string &m) :\n> > +       TestStatusBase(ValueFail, \"not ok\", m)\n> >  {\n> > -       value = ValueFail;\n> > -       prefix = \"not ok \";\n> > -       message = m;\n> >  };\n> >\n> > -TestStatusSkip::TestStatusSkip(const std::string &m)\n> > +TestStatusSkip::TestStatusSkip(const std::string &m) :\n> > +       TestStatusBase(ValueSkip, \"skip\", m)\n> >  {\n> > -       value = ValueSkip;\n> > -       prefix = \"skip \";\n> > -       message = m;\n> >  };\n> > diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n> > index 2e4713a..7f3bb26 100644\n> > --- a/test/libtest/test_status.h\n> > +++ b/test/libtest/test_status.h\n> > @@ -7,17 +7,14 @@\n> >  #ifndef __TEST_TEST_STATUS_H__\n> >  #define __TEST_TEST_STATUS_H__\n> >\n> > -#include <iostream>\n> >  #include <string>\n> >\n> >  class TestStatusBase\n> >  {\n> >  public:\n> > -       TestStatusBase();\n> > -       TestStatusBase(const std::string &m);\n> >         ~TestStatusBase();\n> >\n> > -       operator int() { return value; };\n> > +       operator int() { return result_; };\n> >\n> >         enum ReturnValues {\n> >                 ValuePass = 0,\n> > @@ -26,9 +23,12 @@ public:\n> >         };\n> >\n> >  protected:\n> > -       int value;\n> > -       std::string prefix;\n> >  public:\n> > -       TestStatusBase();\n> > -       TestStatusBase(const std::string &m);\n> >         ~TestStatusBase();\n> >\n> > -       operator int() { return value; };\n> > +       operator int() { return result_; };\n> >\n> >         enum ReturnValues {\n> >                 ValuePass = 0,\n> > @@ -26,9 +23,12 @@ public:\n> >         };\n> >\n> >  protected:\n> > -       int value;\n> > -       std::string prefix;\n> > -       std::string message;\n> > +       TestStatusBase() = delete;\n> > +       TestStatusBase(enum ReturnValues result, const std::string &prefix,\n> > +                      const std::string &m);\n>\n> > +       enum ReturnValues result_;\n>\n> Ah - yes - correct value type is a good call too :)\n>\n> > +       std::string prefix_;\n> > +       std::string message_;\n> >  };\n> >\n> >  class TestStatusPass : public TestStatusBase\n> >\n> >\n> >> --\n> >> 2.17.1\n> >>\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 366B560C79\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 14:35:18 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 9B8C540002;\n\tMon, 14 Jan 2019 13:35:17 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 14 Jan 2019 14:35:27 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190114133527.sguw6vpm3b7bkm6k@uno.localdomain>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114095417.16473-2-kieran.bingham@ideasonboard.com>\n\t<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>\n\t<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qaoh6hbjjjqvansa\"","Content-Disposition":"inline","In-Reply-To":"<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 13:35:18 -0000"}},{"id":304,"web_url":"https://patchwork.libcamera.org/comment/304/","msgid":"<193e8734-e019-86ab-c59c-c49e01fb519a@ideasonboard.com>","date":"2019-01-14T13:40:27","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\n\nOn 14/01/2019 13:35, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Mon, Jan 14, 2019 at 01:24:49PM +0000, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n>> Thanks - Shall I take your detailed review comments as a good indicator\n>> that you like this direction?\n>>\n> \n> Yes I do :)\n\nGreat...\n\n> \n>> I'm sorry it was not so polished, (hence the RFC) - I wanted to gauge\n>> feel on if this is a good way forwards as much as showing the\n>> implementation.\n>>\n>> That said - you've fixed up the initialiser for the three classes for me :)\n>>\n>> So now I can use:\n>>\n>> TestPass::TestPass(const std::string &m) :\n>>      TestStatus(ValuePass, \"ok\", m) {}\n> \n> great, but please not my below patches do not respect the \"TAP\" output\n> format, I just found out what was about after sending this.\n> \n> How do we plan to use the TAP formatted output?\n\nNot entirely sure yet - we're still in RFC :)\n\nLaurent and Niklas were keen to move in that direction - and either way\n- this RFC moves towards making sure each test states what fails when\nsomething goes wrong and introduces a way to start outputting some form\nof Logging ... so it's got merit even without something parsing the\nresults at the end yet....\n\nI assume there are a whole bunch of tools that read TAP.\n  (it's not complex anyway)\n\n\n--\nKieran\n\n> \n> Thanks\n>    j\n> \n>>\n>>\n>>\n>> On 14/01/2019 10:46, Jacopo Mondi wrote:\n>>> Hi Kieran,\n>>>\n>>> On Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:\n>>>> Provide Class object to return the test status, and perform any result reporting.\n>>>>\n>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>> ---\n>>>>  test/libtest/meson.build     |  1 +\n>>>>  test/libtest/test.h          |  2 ++\n>>>>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n>>>>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n>>>>  4 files changed, 100 insertions(+)\n>>>>  create mode 100644 test/libtest/test_status.cpp\n>>>>  create mode 100644 test/libtest/test_status.h\n>>>>\n>>>> diff --git a/test/libtest/meson.build b/test/libtest/meson.build\n>>>> index e0893b70c3d4..a9e67d8d7e6c 100644\n>>>> --- a/test/libtest/meson.build\n>>>> +++ b/test/libtest/meson.build\n>>>> @@ -1,5 +1,6 @@\n>>>>  libtest_sources = files([\n>>>>      'test.cpp',\n>>>> +    'test_status.cpp',\n>>>>  ])\n>>>>\n>>>>  libtest = static_library('libtest', libtest_sources)\n>>>> diff --git a/test/libtest/test.h b/test/libtest/test.h\n>>>> index ec08bf97c03d..d816cf15aaf0 100644\n>>>> --- a/test/libtest/test.h\n>>>> +++ b/test/libtest/test.h\n>>>> @@ -9,6 +9,8 @@\n>>>>\n>>>>  #include <sstream>\n>>>>\n>>>> +#include \"test_status.h\"\n>>>> +\n>>>>  enum TestStatus {\n>>>>  \tTestPass = 0,\n>>>>  \tTestFail = -1,\n>>>\n>>> When you will replace this, please move it to test_status.h\n>>\n>> They already are - they are there as ValuePass. I aim to rename the\n>> class TestStatusPass() to class TestPass() and thus this enum TestStatus\n>> shall then be removed completely.\n>>\n>> I've kept it during this RFC to allow both to live side by side for\n>> trying out the new style - and providing comments :)\n>>\n>>>\n>>>> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..c0daef866740\n>>>> --- /dev/null\n>>>> +++ b/test/libtest/test_status.cpp\n>>>> @@ -0,0 +1,42 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2019, Google Inc.\n>>>> + *\n>>>> + * test_status.cpp - libcamera test result management\n>>>> + */\n>>>> +\n>>>> +#include \"test.h\"\n>>>> +\n>>>> +static unsigned int test_number = 0;\n>>>> +\n>>>> +TestStatusBase::TestStatusBase()\n>>>> +\t: value(-99)\n>>>\n>>> -99 ?\n>>\n>> I needed a random non-defined number. TestStatusBase shouldn't be\n>> instantiated.\n>>\n>>\n>>>> +{\n>>>> +\ttest_number++;\n>>>> +};\n>>>> +\n>>>> +TestStatusBase::~TestStatusBase()\n>>>> +{\n>>>> +\tstd::cout << prefix << test_number << \" \" << message << std::endl;\n>>>\n>>> cout or cerr? This has been asked already bu never finalized\n>>\n>> Well the best part about this series is that there will be only one\n>> place to change when we decide :)\n>>\n>> cout will be fine for the moment I think.\n>>\n>>\n>>>\n>>>> +};\n>>>> +\n>>>> +TestStatusPass::TestStatusPass(const std::string &m)\n>>>> +{\n>>>> +\tvalue = ValuePass;\n>>>> +\tprefix = \"ok \";\n>>>> +\tmessage = m;\n>>>> +};\n>>>> +\n>>>> +TestStatusFail::TestStatusFail(const std::string &m)\n>>>> +{\n>>>> +\tvalue = ValueFail;\n>>>> +\tprefix = \"not ok \";\n>>>> +\tmessage = m;\n>>>> +};\n>>>> +\n>>>> +TestStatusSkip::TestStatusSkip(const std::string &m)\n>>>> +{\n>>>> +\tvalue = ValueSkip;\n>>>> +\tprefix = \"skip \";\n>>>> +\tmessage = m;\n>>>> +};\n>>>\n>>> Use the base constructor, as noted below in a comment to the header\n>>> file\n>>\n>> Thank you for the patch -  I tried to do this through the initialiser\n>> list - and couldn't get it to work correctly.\n>>\n>> Now I see I was missing the wrapping for the TestBase() - thanks I'll\n>> update.\n>>\n>>\n>>\n>>>\n>>>> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n>>>> new file mode 100644\n>>>> index 000000000000..2e4713ad3f6d\n>>>> --- /dev/null\n>>>> +++ b/test/libtest/test_status.h\n>>>> @@ -0,0 +1,55 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2019, Google Inc.\n>>>> + *\n>>>> + * test_status.h - libcamera test status class\n>>>\n>>> Make this and the .cpp one the same\n>>>\n>>>> + */\n>>>> +#ifndef __TEST_TEST_STATUS_H__\n>>>> +#define __TEST_TEST_STATUS_H__\n>>>> +\n>>>> +#include <iostream>\n>>>\n>>> You can move this to the cpp file\n>>>\n>>>> +#include <string>\n>>>> +\n>>>> +class TestStatusBase\n>>>> +{\n>>>> +public:\n>>>> +\tTestStatusBase();\n>>>> +\tTestStatusBase(const std::string &m);\n>>>\n>>> TestStatusBase() is not used and shall be deleted\n>>> TestStatusBase(const std::string m) should be protected as this base\n>>> class shall not be instantiated directly\n>>\n>> Correct :)\n>>\n>>\n>>>\n>>>> +\t~TestStatusBase();\n>>>> +\n>>>> +\toperator int() { return value; };\n>>>> +\n>>>> +\tenum ReturnValues {\n>>>> +\t\tValuePass = 0,\n>>>> +\t\tValueFail = -1,\n>>>> +\t\tValueSkip = 77,\n>>>> +\t};\n>>>\n>>> This and TestStatus can be unified\n>>\n>> Not quite - I want to use the TestPass TestSkip, TestFail namings for\n>> the actual object names.\n>>\n>>\n>>>\n>>>> +\n>>>> +protected:\n>>>> +\tint value;\n>>>> +\tstd::string prefix;\n>>>> +\tstd::string message;\n>>>\n>>> class member end with _\n>>>> +};\n>>>> +\n>>>> +class TestStatusPass : public TestStatusBase\n>>>\n>>> All of these shall use their base constructor\n>>>> +{\n>>>> +public:\n>>>> +\tTestStatusPass(const std::string &m);\n>>>> +};\n>>>> +\n>>>> +class TestStatusFail : public TestStatusBase\n>>>> +{\n>>>> +public:\n>>>> +\tTestStatusFail(const std::string &m);\n>>>> +};\n>>>> +\n>>>> +class TestStatusSkip : public TestStatusBase\n>>>> +{\n>>>> +public:\n>>>> +\tTestStatusSkip(const std::string &m);\n>>>> +};\n>>>> +\n>>>> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n>>>> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) : TestStatusFail((m));})\n>>>> +\n>>>> +#endif /* __TEST_TEST_STATUS_H__ */\n>>>\n>>> Here it is a patch on top that addresses most of my comments and that\n>>> produces this output when run through the test_status_test\n>>>\n>>> /home/jmondi/project/libcamera/libcamera.git/build/test/test_status\n>>> --- stdout ---\n>>> Test: 1: ok - [Verify TestStatusPass]\n>>> Test: 2: not ok - [Verify TestStatusFail]\n>>> Test: 3: skip - [Verify TestStatusSkip]\n>>> Test: 4: ok - [Good is return check]\n>>> Test: 5: ok - [Good isn't return check]\n>>> Test: 6: not ok - [Bad Is Check]\n>>> Test: 7: not ok - [Bad Isn't check]\n>>> Test: 8: ok - TestStatus validations\n>>> -------\n>>>\n>>> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp\n>>> index c0daef8..be84556 100644\n>>> --- a/test/libtest/test_status.cpp\n>>> +++ b/test/libtest/test_status.cpp\n>>> @@ -5,38 +5,37 @@\n>>>   * test_status.cpp - libcamera test result management\n>>>   */\n>>>\n>>> +#include <iostream>\n>>> +#include <string>\n>>> +\n>>>  #include \"test.h\"\n>>>\n>>>  static unsigned int test_number = 0;\n>>>\n>>> -TestStatusBase::TestStatusBase()\n>>> -       : value(-99)\n>>> +TestStatusBase::TestStatusBase(enum ReturnValues result,\n>>> +                              const std::string &prefix,\n>>> +                              const std::string &message)\n>>> +       : result_(result), prefix_(prefix), message_(message)\n>>>  {\n>>>         test_number++;\n>>>  };\n>>>\n>>>  TestStatusBase::~TestStatusBase()\n>>>  {\n>>> -       std::cout << prefix << test_number << \" \" << message << std::endl;\n>>> +       std::cout << \"Test: \" << test_number << \": \" << prefix_ << \" - \"\n>>> +                 << message_ << std::endl;\n>>>\n>>>  };\n>>>\n>>> -TestStatusPass::TestStatusPass(const std::string &m)\n>>> +TestStatusPass::TestStatusPass(const std::string &m) :\n>>> +       TestStatusBase(ValuePass, \"ok\", m)\n>>>  {\n>>> -       value = ValuePass;\n>>> -       prefix = \"ok \";\n>>> -       message = m;\n>>>  };\n>>>\n>>> -TestStatusFail::TestStatusFail(const std::string &m)\n>>> +TestStatusFail::TestStatusFail(const std::string &m) :\n>>> +       TestStatusBase(ValueFail, \"not ok\", m)\n>>>  {\n>>> -       value = ValueFail;\n>>> -       prefix = \"not ok \";\n>>> -       message = m;\n>>>  };\n>>>\n>>> -TestStatusSkip::TestStatusSkip(const std::string &m)\n>>> +TestStatusSkip::TestStatusSkip(const std::string &m) :\n>>> +       TestStatusBase(ValueSkip, \"skip\", m)\n>>>  {\n>>> -       value = ValueSkip;\n>>> -       prefix = \"skip \";\n>>> -       message = m;\n>>>  };\n>>> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n>>> index 2e4713a..7f3bb26 100644\n>>> --- a/test/libtest/test_status.h\n>>> +++ b/test/libtest/test_status.h\n>>> @@ -7,17 +7,14 @@\n>>>  #ifndef __TEST_TEST_STATUS_H__\n>>>  #define __TEST_TEST_STATUS_H__\n>>>\n>>> -#include <iostream>\n>>>  #include <string>\n>>>\n>>>  class TestStatusBase\n>>>  {\n>>>  public:\n>>> -       TestStatusBase();\n>>> -       TestStatusBase(const std::string &m);\n>>>         ~TestStatusBase();\n>>>\n>>> -       operator int() { return value; };\n>>> +       operator int() { return result_; };\n>>>\n>>>         enum ReturnValues {\n>>>                 ValuePass = 0,\n>>> @@ -26,9 +23,12 @@ public:\n>>>         };\n>>>\n>>>  protected:\n>>> -       int value;\n>>> -       std::string prefix;\n>>>  public:\n>>> -       TestStatusBase();\n>>> -       TestStatusBase(const std::string &m);\n>>>         ~TestStatusBase();\n>>>\n>>> -       operator int() { return value; };\n>>> +       operator int() { return result_; };\n>>>\n>>>         enum ReturnValues {\n>>>                 ValuePass = 0,\n>>> @@ -26,9 +23,12 @@ public:\n>>>         };\n>>>\n>>>  protected:\n>>> -       int value;\n>>> -       std::string prefix;\n>>> -       std::string message;\n>>> +       TestStatusBase() = delete;\n>>> +       TestStatusBase(enum ReturnValues result, const std::string &prefix,\n>>> +                      const std::string &m);\n>>\n>>> +       enum ReturnValues result_;\n>>\n>> Ah - yes - correct value type is a good call too :)\n>>\n>>> +       std::string prefix_;\n>>> +       std::string message_;\n>>>  };\n>>>\n>>>  class TestStatusPass : public TestStatusBase\n>>>\n>>>\n>>>> --\n>>>> 2.17.1\n>>>>\n>>>> _______________________________________________\n>>>> libcamera-devel mailing list\n>>>> libcamera-devel@lists.libcamera.org\n>>>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62E6760C7D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 14:40:30 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 92D55530;\n\tMon, 14 Jan 2019 14:40:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547473229;\n\tbh=bXe6sslOJ2+A/HA9LmdYOr4YxvnmnYzOacUYyF+CgVA=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=VrLxM2wF/bfrn7m60pfHHXwunk2EYQGH8HrEWgU/+fKGVpSYr/dpuZ4E18YR2zf1h\n\tOBp0mEbuzL9d4SWs3btmej5CJnjopNKZwofAfgko9imq3LtoBYCTfBneCyybLtcKkq\n\tc0jFwpQNBHE0OycPbwvSsLuBlfk9JS+o1yhdp/Ls=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114095417.16473-2-kieran.bingham@ideasonboard.com>\n\t<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>\n\t<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>\n\t<20190114133527.sguw6vpm3b7bkm6k@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<193e8734-e019-86ab-c59c-c49e01fb519a@ideasonboard.com>","Date":"Mon, 14 Jan 2019 13:40:27 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20190114133527.sguw6vpm3b7bkm6k@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 13:40:30 -0000"}},{"id":309,"web_url":"https://patchwork.libcamera.org/comment/309/","msgid":"<3043996.WmpEjsec8I@avalon>","date":"2019-01-14T17:33:27","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Monday, 14 January 2019 15:24:49 EET Kieran Bingham wrote:\n> Hi Jacopo,\n> \n> Thanks - Shall I take your detailed review comments as a good indicator\n> that you like this direction?\n> \n> I'm sorry it was not so polished, (hence the RFC) - I wanted to gauge\n> feel on if this is a good way forwards as much as showing the\n> implementation.\n> \n> That said - you've fixed up the initialiser for the three classes for me :)\n> \n> So now I can use:\n> \n> TestPass::TestPass(const std::string &m) :\n>      TestStatus(ValuePass, \"ok\", m) {}\n\nThat looks much better to me too.\n\nWhile there is interesting code in this series, it starts to blur the line \nbetween different test-related concepts without defining them. The Test class \nwas originally intended to model a test, and now you introduce some kind of \nsub-test concept. I think this should be documented in order to discuss the \ndesign, with a clear explanation of what tests, test suites, test cases, unit \ntests, test sets and any other concept we may need is. It's hard to review \nthis series without knowing exactly what you have in mind, and I think that \nad-hoc extensions to the test infrastructure will build a technical debt we'll \nhave a hard time repaying.\n\n> On 14/01/2019 10:46, Jacopo Mondi wrote:\n> > On Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:\n> >> Provide Class object to return the test status, and perform any result\n> >> reporting.\n> >> \n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >> ---\n> >> \n> >>  test/libtest/meson.build     |  1 +\n> >>  test/libtest/test.h          |  2 ++\n> >>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n> >>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n> >>  4 files changed, 100 insertions(+)\n> >>  create mode 100644 test/libtest/test_status.cpp\n> >>  create mode 100644 test/libtest/test_status.h","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A66A160C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 18:32:12 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02B6A530;\n\tMon, 14 Jan 2019 18:32:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547487132;\n\tbh=A/H4qCUukBu3ZkWYywWbzGMaXNPf4cdx5eWnsfFhOTY=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=PtHYuOq6Fxc7Vxv68sRKJUV5k0rbN+Td01hdU7lij9Sczr1IZ+n2eZzonP9+VNIav\n\tJ1u9vxOF5Ig7DWqGP1BNUJrgJiG1wWazc73ATcujr8F8PIpiiINKXSKOzsKRhDpa+H\n\tkHIByt8FqcVOTOPcCDlPCnKuQ7ezYFM3tX+N7fpE=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org, kieran.bingham@ideasonboard.com","Date":"Mon, 14 Jan 2019 19:33:27 +0200","Message-ID":"<3043996.WmpEjsec8I@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114104604.vhxdvmwd4ng4pzvj@uno.localdomain>\n\t<b331528a-5dda-9c2b-ffc2-2bb47045ce99@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 17:32:12 -0000"}},{"id":310,"web_url":"https://patchwork.libcamera.org/comment/310/","msgid":"<1801061.FgutEGnknN@avalon>","date":"2019-01-14T17:38:49","subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Monday, 14 January 2019 11:54:16 EET Kieran Bingham wrote:\n> Provide Class object to return the test status, and perform any result\n> reporting.\n> \n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  test/libtest/meson.build     |  1 +\n>  test/libtest/test.h          |  2 ++\n>  test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++\n>  test/libtest/test_status.h   | 55 ++++++++++++++++++++++++++++++++++++\n>  4 files changed, 100 insertions(+)\n>  create mode 100644 test/libtest/test_status.cpp\n>  create mode 100644 test/libtest/test_status.h\n\n[snip]\n\n> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h\n> new file mode 100644\n> index 000000000000..2e4713ad3f6d\n> --- /dev/null\n> +++ b/test/libtest/test_status.h\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * test_status.h - libcamera test status class\n> + */\n> +#ifndef __TEST_TEST_STATUS_H__\n> +#define __TEST_TEST_STATUS_H__\n> +\n> +#include <iostream>\n> +#include <string>\n> +\n> +class TestStatusBase\n> +{\n> +public:\n> +\tTestStatusBase();\n> +\tTestStatusBase(const std::string &m);\n> +\t~TestStatusBase();\n> +\n> +\toperator int() { return value; };\n> +\n> +\tenum ReturnValues {\n> +\t\tValuePass = 0,\n> +\t\tValueFail = -1,\n> +\t\tValueSkip = 77,\n> +\t};\n> +\n> +protected:\n> +\tint value;\n> +\tstd::string prefix;\n> +\tstd::string message;\n> +};\n> +\n> +class TestStatusPass : public TestStatusBase\n> +{\n> +public:\n> +\tTestStatusPass(const std::string &m);\n\nI think we need a way to log more complex messages with parameters.\n\n> +};\n> +\n> +class TestStatusFail : public TestStatusBase\n> +{\n> +public:\n> +\tTestStatusFail(const std::string &m);\n> +};\n> +\n> +class TestStatusSkip : public TestStatusBase\n> +{\n> +public:\n> +\tTestStatusSkip(const std::string &m);\n> +};\n> +\n> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) :\n> TestStatusFail((m));})\n> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) :\n> TestStatusFail((m));})\n\nI would prefer names along the lines of assertEqual() and assertNotEqual(), as \nthat would make it easier to add other helpers such as assertSmallerThan() for \ninstance (whether we will want to do so remains to be decided).\n\nI'm also concerned by the is() and isnt() macros closing a test (as in \nreturning either pass or fail), as a single test may require multiple checks \nto decide on the result. I think this boils down to defining the test-related \nconcepts we want to use (test case, test suite, unit test, ...) as explained \nin my other reply to this series, and then building on top of that. If we try \nto write code without clearly defining the direction we want to take it will \nbe painful at best.\n\n> +\n> +#endif /* __TEST_TEST_STATUS_H__ */","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 108F860C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jan 2019 18:37:34 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 81897530;\n\tMon, 14 Jan 2019 18:37:33 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547487453;\n\tbh=9DnHi2Tg0dxC4Hq34n77WKi74IlfTbFdQWXCh1aRL1g=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=T+WXHI+1Fszf47hGnOxO4KSEiJF1CgsgvTXy4kn9aVRTDO5TJyaLgYIelAGSjInx+\n\tzYGm+6KTUQxCb10bFs8p8q46M1VdqJ2RVdtv8o+/+3vDvuTkKnmHC+k6E5b8jheig3\n\tgUOOUFki6KOEmTnW/YX4MJVVJNu6bXlwdlthcOv0=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 14 Jan 2019 19:38:49 +0200","Message-ID":"<1801061.FgutEGnknN@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190114095417.16473-2-kieran.bingham@ideasonboard.com>","References":"<20190114095417.16473-1-kieran.bingham@ideasonboard.com>\n\t<20190114095417.16473-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 14 Jan 2019 17:37:34 -0000"}}]