[libcamera-devel] [PATCH RFC 2/2] test: Provide TestStatus validation tests

[Kieran Bingham]

Hi Jacopo,

On 14/01/2019 10:51, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Mon, Jan 14, 2019 at 09:54:17AM +0000, Kieran Bingham wrote:
>> Validate the return values of the objects, and the ability to use "is"
>> and "isnt()" correctly.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  test/meson.build     |  1 +
>>  test/test_status.cpp | 52 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+)
>>  create mode 100644 test/test_status.cpp
>>
>> diff --git a/test/meson.build b/test/meson.build
>> index 32152888a55e..ae5bd7b47b3b 100644
>> --- a/test/meson.build
>> +++ b/test/meson.build
>> @@ -6,6 +6,7 @@ public_tests = [
>>      ['event',           'event.cpp'],
>>      ['list-cameras',    'list-cameras.cpp'],
>>      ['signal',          'signal.cpp'],
>> +    ['test_status',     'test_status.cpp'],
>>      ['timer',           'timer.cpp'],
>>  ]
>>
>> diff --git a/test/test_status.cpp b/test/test_status.cpp
>> new file mode 100644
>> index 000000000000..297cc5189f49
>> --- /dev/null
>> +++ b/test/test_status.cpp
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * list.cpp - camera list tests
> 
> I know where this comes from
> 
>> + */
>> +
>> +#include <iostream>
>> +#include <string>
> 
> Do you need these?
> 
>> +
>> +#include "test.h"
>> +
>> +class TestStatusTest : public Test
>> +{
>> +protected:
>> +	int run()
>> +	{
>> +		// plan(8);
>> +		// note("Correct output here is 1 skip, and three failures");
> 
> Please drop there

Actually these are discussion items :)

in TAP you are supposed to define how many tests there are.

I've added a static counter which keeps track internally - but this has
a big flaw...

It can only increment if there is an instantiation of the
TestStatusBase() object...

so ... imagine the below:


 if ( 1 != 1 ) {
	TestFail("Equality Failure");
 }
 ...

 if (a == a) {
	TestPass("Equality Success");
 }
	


In the TestPass scenario the internal counters will indicate that a test
occured - (assuming a==a), but the TestFail() instance will not increase
the global counter.

So - that would imply that for any test or check there should always be
an instance of TestPass(), TestSkip(), or TestFail().

The is() / isnt() macro's do this by always returning Pass or Fail ...
but there's scope that we might add extras not usign such macros.

Anyway - thoughts on a postcard here please :) As I said - it's a
discussion topic ...



> 
>> +
>> +		/*
>> +		 * TestStatusPass should return 0.
>> +		 * Test without operator=
>> +		 */
>> +		if (TestStatusPass("[Verify TestStatusPass]"))
>> +			return TestStatusFail("TestStatusPass test");
>> +
>> +		/* Test an integer on the rhs. */
>> +		if (TestStatusFail("[Verify TestStatusFail]") != -1)
>> +			return TestStatusFail("TestStatusFail test");
>> +
>> +		/* Test an integer on the lhs. */
>> +		if (77 != TestStatusSkip("[Verify TestStatusSkip]"))
>> +			return TestStatusFail("TestStatusSkip test");
>> +
>> +		if (is(1, 1, "[Good is return check]") != TestStatusBase::ValuePass)
>> +			return TestStatusFail("Good is check failed");
> 
> Please use '' around is, otherwise the result is very confusing

That's why I wrapped the tested parts with "[" and "]" ... because we're
creating test objects which we then ... test... so it's a bit
self-consuming here.

Because of that - I'm not even sure that this test should live in the
suite - as it will always create a 'fake' failure ... which we might not
want in our test outputs.

But it was all here to demonstrate the usage and make sure they did the
right thing ...


> 
> Test: 4: ok - [Good 'is()' return check]
> 
> I would actually write is as:
> "'is()' predicated test successful"
> 
>> +
>> +		if (isnt(1, 0, "[Good isn't return check]") != TestStatusBase::ValuePass)
>> +			return TestStatusFail("Good isn't check failed");
> 
> same here and below
> 
>> +
>> +		if (is(1, 0, "[Bad Is Check]") == TestStatusBase::ValuePass)
>> +			return TestStatusFail("Bad is check failed");
>> +
>> +		if (isnt(1, 1, "[Bad Isn't check]") == TestStatusBase::ValuePass)
>> +			return TestStatusFail("Bad isn't check failed");
>> +
>> +		return TestStatusPass("TestStatus validations");
> 
> Could you make tests where the message output is created by
> concatenating strings? I assume will be used a lot in practice..
> 
> 		std::string tmp="a string";
> 		if (TestStatusPass("[Verify " + tmp + " TestStatusPass]"))
> 			return TestStatusFail("TestStatusPass test");

Great - another discussion topic ... how do we handle complex strings
for the reporting.

I wonder if we can play around with the << operator to allow it to act
like a Log() object?

 	return TestFail() << "Device : " << dev_ << " didn't work";

I see that you've used lots of << operators on your test results outputs...


> 
> Thanks
>   j
> 
>> +	}
>> +};
>> +
>> +TEST_REGISTER(TestStatusTest)
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran