[libcamera-devel] [PATCH RFC 1/2] test: Add TestStatus classes
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 14 14:35:27 CET 2019
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 :)
> 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?
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190114/1e62be90/attachment.sig>
More information about the libcamera-devel
mailing list