[libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 14 14:24:49 CET 2019
Hi Jacopo,
Thanks - Shall I take your detailed review comments as a good indicator
that you like this direction?
I'm sorry it was not so polished, (hence the RFC) - I wanted to gauge
feel on if this is a good way forwards as much as showing the
implementation.
That said - you've fixed up the initialiser for the three classes for me :)
So now I can use:
TestPass::TestPass(const std::string &m) :
TestStatus(ValuePass, "ok", m) {}
On 14/01/2019 10:46, Jacopo Mondi wrote:
> Hi Kieran,
>
> On Mon, Jan 14, 2019 at 09:54:16AM +0000, Kieran Bingham wrote:
>> Provide Class object to return the test status, and perform any result reporting.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> test/libtest/meson.build | 1 +
>> test/libtest/test.h | 2 ++
>> test/libtest/test_status.cpp | 42 +++++++++++++++++++++++++++
>> test/libtest/test_status.h | 55 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 100 insertions(+)
>> create mode 100644 test/libtest/test_status.cpp
>> create mode 100644 test/libtest/test_status.h
>>
>> diff --git a/test/libtest/meson.build b/test/libtest/meson.build
>> index e0893b70c3d4..a9e67d8d7e6c 100644
>> --- a/test/libtest/meson.build
>> +++ b/test/libtest/meson.build
>> @@ -1,5 +1,6 @@
>> libtest_sources = files([
>> 'test.cpp',
>> + 'test_status.cpp',
>> ])
>>
>> libtest = static_library('libtest', libtest_sources)
>> diff --git a/test/libtest/test.h b/test/libtest/test.h
>> index ec08bf97c03d..d816cf15aaf0 100644
>> --- a/test/libtest/test.h
>> +++ b/test/libtest/test.h
>> @@ -9,6 +9,8 @@
>>
>> #include <sstream>
>>
>> +#include "test_status.h"
>> +
>> enum TestStatus {
>> TestPass = 0,
>> TestFail = -1,
>
> When you will replace this, please move it to test_status.h
They already are - they are there as ValuePass. I aim to rename the
class TestStatusPass() to class TestPass() and thus this enum TestStatus
shall then be removed completely.
I've kept it during this RFC to allow both to live side by side for
trying out the new style - and providing comments :)
>
>> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp
>> new file mode 100644
>> index 000000000000..c0daef866740
>> --- /dev/null
>> +++ b/test/libtest/test_status.cpp
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * test_status.cpp - libcamera test result management
>> + */
>> +
>> +#include "test.h"
>> +
>> +static unsigned int test_number = 0;
>> +
>> +TestStatusBase::TestStatusBase()
>> + : value(-99)
>
> -99 ?
I needed a random non-defined number. TestStatusBase shouldn't be
instantiated.
>> +{
>> + test_number++;
>> +};
>> +
>> +TestStatusBase::~TestStatusBase()
>> +{
>> + std::cout << prefix << test_number << " " << message << std::endl;
>
> cout or cerr? This has been asked already bu never finalized
Well the best part about this series is that there will be only one
place to change when we decide :)
cout will be fine for the moment I think.
>
>> +};
>> +
>> +TestStatusPass::TestStatusPass(const std::string &m)
>> +{
>> + value = ValuePass;
>> + prefix = "ok ";
>> + message = m;
>> +};
>> +
>> +TestStatusFail::TestStatusFail(const std::string &m)
>> +{
>> + value = ValueFail;
>> + prefix = "not ok ";
>> + message = m;
>> +};
>> +
>> +TestStatusSkip::TestStatusSkip(const std::string &m)
>> +{
>> + value = ValueSkip;
>> + prefix = "skip ";
>> + message = m;
>> +};
>
> Use the base constructor, as noted below in a comment to the header
> file
Thank you for the patch - I tried to do this through the initialiser
list - and couldn't get it to work correctly.
Now I see I was missing the wrapping for the TestBase() - thanks I'll
update.
>
>> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h
>> new file mode 100644
>> index 000000000000..2e4713ad3f6d
>> --- /dev/null
>> +++ b/test/libtest/test_status.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * test_status.h - libcamera test status class
>
> Make this and the .cpp one the same
>
>> + */
>> +#ifndef __TEST_TEST_STATUS_H__
>> +#define __TEST_TEST_STATUS_H__
>> +
>> +#include <iostream>
>
> You can move this to the cpp file
>
>> +#include <string>
>> +
>> +class TestStatusBase
>> +{
>> +public:
>> + TestStatusBase();
>> + TestStatusBase(const std::string &m);
>
> TestStatusBase() is not used and shall be deleted
> TestStatusBase(const std::string m) should be protected as this base
> class shall not be instantiated directly
Correct :)
>
>> + ~TestStatusBase();
>> +
>> + operator int() { return value; };
>> +
>> + enum ReturnValues {
>> + ValuePass = 0,
>> + ValueFail = -1,
>> + ValueSkip = 77,
>> + };
>
> This and TestStatus can be unified
Not quite - I want to use the TestPass TestSkip, TestFail namings for
the actual object names.
>
>> +
>> +protected:
>> + int value;
>> + std::string prefix;
>> + std::string message;
>
> class member end with _
>> +};
>> +
>> +class TestStatusPass : public TestStatusBase
>
> All of these shall use their base constructor
>> +{
>> +public:
>> + TestStatusPass(const std::string &m);
>> +};
>> +
>> +class TestStatusFail : public TestStatusBase
>> +{
>> +public:
>> + TestStatusFail(const std::string &m);
>> +};
>> +
>> +class TestStatusSkip : public TestStatusBase
>> +{
>> +public:
>> + TestStatusSkip(const std::string &m);
>> +};
>> +
>> +#define is(a, b, m) ({((a) == (b)) ? TestStatusPass((m)) : TestStatusFail((m));})
>> +#define isnt(a, b, m) ({((a) != (b)) ? TestStatusPass((m)) : TestStatusFail((m));})
>> +
>> +#endif /* __TEST_TEST_STATUS_H__ */
>
> Here it is a patch on top that addresses most of my comments and that
> produces this output when run through the test_status_test
>
> /home/jmondi/project/libcamera/libcamera.git/build/test/test_status
> --- stdout ---
> Test: 1: ok - [Verify TestStatusPass]
> Test: 2: not ok - [Verify TestStatusFail]
> Test: 3: skip - [Verify TestStatusSkip]
> Test: 4: ok - [Good is return check]
> Test: 5: ok - [Good isn't return check]
> Test: 6: not ok - [Bad Is Check]
> Test: 7: not ok - [Bad Isn't check]
> Test: 8: ok - TestStatus validations
> -------
>
> diff --git a/test/libtest/test_status.cpp b/test/libtest/test_status.cpp
> index c0daef8..be84556 100644
> --- a/test/libtest/test_status.cpp
> +++ b/test/libtest/test_status.cpp
> @@ -5,38 +5,37 @@
> * test_status.cpp - libcamera test result management
> */
>
> +#include <iostream>
> +#include <string>
> +
> #include "test.h"
>
> static unsigned int test_number = 0;
>
> -TestStatusBase::TestStatusBase()
> - : value(-99)
> +TestStatusBase::TestStatusBase(enum ReturnValues result,
> + const std::string &prefix,
> + const std::string &message)
> + : result_(result), prefix_(prefix), message_(message)
> {
> test_number++;
> };
>
> TestStatusBase::~TestStatusBase()
> {
> - std::cout << prefix << test_number << " " << message << std::endl;
> + std::cout << "Test: " << test_number << ": " << prefix_ << " - "
> + << message_ << std::endl;
>
> };
>
> -TestStatusPass::TestStatusPass(const std::string &m)
> +TestStatusPass::TestStatusPass(const std::string &m) :
> + TestStatusBase(ValuePass, "ok", m)
> {
> - value = ValuePass;
> - prefix = "ok ";
> - message = m;
> };
>
> -TestStatusFail::TestStatusFail(const std::string &m)
> +TestStatusFail::TestStatusFail(const std::string &m) :
> + TestStatusBase(ValueFail, "not ok", m)
> {
> - value = ValueFail;
> - prefix = "not ok ";
> - message = m;
> };
>
> -TestStatusSkip::TestStatusSkip(const std::string &m)
> +TestStatusSkip::TestStatusSkip(const std::string &m) :
> + TestStatusBase(ValueSkip, "skip", m)
> {
> - value = ValueSkip;
> - prefix = "skip ";
> - message = m;
> };
> diff --git a/test/libtest/test_status.h b/test/libtest/test_status.h
> index 2e4713a..7f3bb26 100644
> --- a/test/libtest/test_status.h
> +++ b/test/libtest/test_status.h
> @@ -7,17 +7,14 @@
> #ifndef __TEST_TEST_STATUS_H__
> #define __TEST_TEST_STATUS_H__
>
> -#include <iostream>
> #include <string>
>
> class TestStatusBase
> {
> public:
> - TestStatusBase();
> - TestStatusBase(const std::string &m);
> ~TestStatusBase();
>
> - operator int() { return value; };
> + operator int() { return result_; };
>
> enum ReturnValues {
> ValuePass = 0,
> @@ -26,9 +23,12 @@ public:
> };
>
> protected:
> - int value;
> - std::string prefix;
> public:
> - TestStatusBase();
> - TestStatusBase(const std::string &m);
> ~TestStatusBase();
>
> - operator int() { return value; };
> + operator int() { return result_; };
>
> enum ReturnValues {
> ValuePass = 0,
> @@ -26,9 +23,12 @@ public:
> };
>
> protected:
> - int value;
> - std::string prefix;
> - std::string message;
> + TestStatusBase() = delete;
> + TestStatusBase(enum ReturnValues result, const std::string &prefix,
> + const std::string &m);
> + enum ReturnValues result_;
Ah - yes - correct value type is a good call too :)
> + std::string prefix_;
> + std::string message_;
> };
>
> class TestStatusPass : public TestStatusBase
>
>
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list