[libcamera-devel] [PATCH 32/31] test: controls: control_value: Expand test to cover all control types

[Laurent Pinchart]

Hi Kieran,

On Thu, Mar 05, 2020 at 05:35:29PM +0000, Kieran Bingham wrote:
> On 01/03/2020 19:26, Laurent Pinchart wrote:
> > The ControlValueTest hasn't been updated for a long time and is
> > outdated. Improve it to support all control types, and test the type(),
> > isArray() and toString() methods.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/controls.cpp      |   2 +-
> >  test/controls/control_value.cpp | 110 +++++++++++++++++++++++++-------
> >  2 files changed, 89 insertions(+), 23 deletions(-)
> > 
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index a9dfc53e8565..aa3e7a267303 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -202,7 +202,7 @@ std::string ControlValue::toString() const
> >  		switch (type_) {
> >  		case ControlTypeBool: {
> >  			const bool *value = reinterpret_cast<const bool *>(data);
> > -			str += *value ? "True" : "False";
> > +			str += *value ? "true" : "false";
> 
> This is not a change under test: and instead modifies the toString
> method of the implementation instead.
> 
> Is that intentional?

Wrong rebase, this belongs to a separate patch.

> I don't see any change below checking this,

Please see below.

> and I don't see a reason to change this to lowercase ...

The C++ boolean types are lower-case, and
std::basic_ostream<CharT,Traits>::operator<<(bool) will produce a
lower-case string (when std::boolalpha is in effect, otherwise it
produces 0 or 1). I thus think lower-case is better.

I'll submit a patch to change this separately from the test.

> >  			break;
> >  		}
> >  		case ControlTypeInteger8: {
> > diff --git a/test/controls/control_value.cpp b/test/controls/control_value.cpp
> > index a1ffa842f29e..111b4c5d1a08 100644
> > --- a/test/controls/control_value.cpp
> > +++ b/test/controls/control_value.cpp
> > @@ -19,46 +19,112 @@ class ControlValueTest : public Test
> >  protected:
> >  	int run()
> >  	{
> > -		ControlValue integer(1234);
> > -		ControlValue boolean(true);
> > +		/*
> > +		 * None type.
> > +		 */
> > +		ControlValue value;
> > +		if (!value.isNone() || value.isArray()) {
> > +			cerr << "Empty value is non-null" << endl;
> > +			return TestFail;
> > +		}
> >  
> > -		/* Just a string conversion output test... no validation */
> > -		cout << "Int: " << integer.toString()
> > -		     << " Bool: " << boolean.toString()
> > -		     << endl;
> > +		/*
> > +		 * Bool type.
> > +		 */
> > +		value.set(true);
> > +		if (value.isNone() || value.isArray() ||
> > +		    value.type() != ControlTypeBool) {
> > +			cerr << "Control type mismatch after setting to bool" << endl;
> > +			return TestFail;
> > +		}
> >  
> > -		if (integer.get<int32_t>() != 1234) {
> > -			cerr << "Failed to get Integer" << endl;
> > +		if (value.get<bool>() != true) {
> > +			cerr << "Control value mismatch after setting to bool" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		if (boolean.get<bool>() != true) {
> > -			cerr << "Failed to get Boolean" << endl;
> > +		if (value.toString() != "true") {

This is the code that tests for lower-case.

> > +			cerr << "Control string mismatch after setting to bool" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		/* Test an uninitialised value, and updating it. */
> > +		/*
> > +		 * Integer8 type.
> > +		 */
> > +		value.set(static_cast<int8_t>(42));
> > +		if (value.isNone() || value.isArray() ||
> > +		    value.type() != ControlTypeInteger8) {
> > +			cerr << "Control type mismatch after setting to int8_t" << endl;
> > +			return TestFail;
> > +		}
> >  
> > -		ControlValue value;
> > -		if (!value.isNone()) {
> > -			cerr << "Empty value is non-null" << endl;
> > +		if (value.get<int8_t>() != 42) {
> > +			cerr << "Control value mismatch after setting to int8_t" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		value.set<bool>(true);
> > -		if (value.isNone()) {
> > -			cerr << "Failed to set an empty object" << endl;
> > +		if (value.toString() != "42") {
> > +			cerr << "Control string mismatch after setting to int8_t" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		if (value.get<bool>() != true) {
> > -			cerr << "Failed to get Booleans" << endl;
> > +		/*
> > +		 * Integer32 type.
> > +		 */
> > +		value.set(0x42000000);
> > +		if (value.isNone() || value.isArray() ||
> > +		    value.type() != ControlTypeInteger32) {
> > +			cerr << "Control type mismatch after setting to int32_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (value.get<int32_t>() != 0x42000000) {
> > +			cerr << "Control value mismatch after setting to int32_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (value.toString() != "1107296256") {
> > +			cerr << "Control string mismatch after setting to int32_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Integer64 type.
> > +		 */
> > +		value.set(static_cast<int64_t>(-42));
> > +		if (value.isNone() || value.isArray() ||
> > +		    value.type() != ControlTypeInteger64) {
> > +			cerr << "Control type mismatch after setting to int64_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (value.get<int64_t>() != -42) {
> > +			cerr << "Control value mismatch after setting to int64_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (value.toString() != "-42") {
> > +			cerr << "Control string mismatch after setting to int64_t" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/*
> > +		 * Float type.
> > +		 */
> > +		value.set(-0.42f);
> > +		if (value.isNone() || value.isArray() ||
> > +		    value.type() != ControlTypeFloat) {
> > +			cerr << "Control type mismatch after setting to float" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		if (value.get<float>() != -0.42f) {
> > +			cerr << "Control value mismatch after setting to float" << endl;
> >  			return TestFail;
> >  		}
> >  
> > -		value.set<int32_t>(10);
> > -		if (value.get<int32_t>() != 10) {
> > -			cerr << "Failed to get Integer" << endl;
> > +		if (value.toString() != "-0.420000") {
> > +			cerr << "Control string mismatch after setting to float" << endl;
> >  			return TestFail;
> >  		}
> >  

-- 
Regards,

Laurent Pinchart