[libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 14 14:40:27 CET 2019
Hi Jacopo,
On 14/01/2019 13:35, Jacopo Mondi wrote:
> Hi Kieran,
>
> On Mon, Jan 14, 2019 at 01:24:49PM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Thanks - Shall I take your detailed review comments as a good indicator
>> that you like this direction?
>>
>
> Yes I do :)
Great...
>
>> 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) {}
>
> great, but please not my below patches do not respect the "TAP" output
> format, I just found out what was about after sending this.
>
> How do we plan to use the TAP formatted output?
Not entirely sure yet - we're still in RFC :)
Laurent and Niklas were keen to move in that direction - and either way
- this RFC moves towards making sure each test states what fails when
something goes wrong and introduces a way to start outputting some form
of Logging ... so it's got merit even without something parsing the
results at the end yet....
I assume there are a whole bunch of tools that read TAP.
(it's not complex anyway)
--
Kieran
>
> Thanks
> j
>
>>
>>
>>
>> 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
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list