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

223 diff mbox series

Validate the return values of the objects, and the ability to use "is"
and "isnt()" correctly.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 test/meson.build     |  1 +
 test/test_status.cpp | 52 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 test/test_status.cpp

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@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

> +
> +		/*
> +		 * 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

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");

Thanks
  j

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

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@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel