[PATCH 09/12] test: timer-thread: Move timer start from wrong thread to separate test

[Milan Zamazal]

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> On Mon, Jan 22, 2024 at 09:17:56PM +0100, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>> 
>
>> > Starting a timer from the wrong thread is expected to fail, and we test
>> > this in the timer-thread unit test. This is however not something that a
>> > caller is allowed to do, and libcamera will get assertion failures to
>> > catch this invalid usage. The unit test will then fail.
>> >
>> > To prepare for this, split the unit test in two, with a test that is
>> > expected by meson to succeed, and one that is expected to fail. The
>> > assertion will then cause an expected failure, making the test suite
>> > succeed.
>> 
>> What if the tests crashes for an unrelated reason?  Is it possible to
>> distinguish between the "right" error and an unexpected error?
>
> Not easily, meson won't distinguish between different assertion errors.
> I'm pretty sure we could create some kind of wrapper that would parse
> the logs, but I don't think that's in scope for this series.

OK.

>> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> > ---
>> >  test/meson.build      |   9 ++--
>> >  test/timer-fail.cpp   | 101 ++++++++++++++++++++++++++++++++++++++++++
>> >  test/timer-thread.cpp |  22 ---------
>> >  3 files changed, 107 insertions(+), 25 deletions(-)
>> >  create mode 100644 test/timer-fail.cpp
>> >
>> > diff --git a/test/meson.build b/test/meson.build
>> > index 189e1428485a..8b6057d4e800 100644
>> > --- a/test/meson.build
>> > +++ b/test/meson.build
>> > @@ -69,6 +69,7 @@ internal_tests = [
>> >      {'name': 'signal-threads', 'sources': ['signal-threads.cpp']},
>> >      {'name': 'threads', 'sources': 'threads.cpp', 'dependencies': [libthreads]},
>> >      {'name': 'timer', 'sources': ['timer.cpp']},
>> > +    {'name': 'timer-fail', 'sources': ['timer-fail.cpp'], 'should_fail': true},
>> >      {'name': 'timer-thread', 'sources': ['timer-thread.cpp']},
>> >      {'name': 'unique-fd', 'sources': ['unique-fd.cpp']},
>> >      {'name': 'utils', 'sources': ['utils.cpp']},
>> > @@ -91,7 +92,7 @@ foreach test : public_tests
>> >                       link_with : test_libraries,
>> >                       include_directories : test_includes_public)
>> >  
>> > -    test(test['name'], exe)
>> > +    test(test['name'], exe, should_fail : test.get('should_fail', false))
>> >  endforeach
>> >  
>> >  foreach test : internal_tests
>> > @@ -105,7 +106,7 @@ foreach test : internal_tests
>> >                       link_with : test_libraries,
>> >                       include_directories : test_includes_internal)
>> >  
>> > -    test(test['name'], exe)
>> > +    test(test['name'], exe, should_fail : test.get('should_fail', false))
>> >  endforeach
>> >  
>> >  foreach test : internal_non_parallel_tests
>> > @@ -119,5 +120,7 @@ foreach test : internal_non_parallel_tests
>> >                       link_with : test_libraries,
>> >                       include_directories : test_includes_internal)
>> >  
>> > -    test(test['name'], exe, is_parallel : false)
>> > +    test(test['name'], exe,
>> > +         is_parallel : false,
>> > +         should_fail : test.get('should_fail', false))
>> >  endforeach
>> > diff --git a/test/timer-fail.cpp b/test/timer-fail.cpp
>> > new file mode 100644
>> > index 000000000000..e9a3b1b61bcb
>> > --- /dev/null
>> > +++ b/test/timer-fail.cpp
>> > @@ -0,0 +1,101 @@
>> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> > +/*
>> > + * Copyright (C) 2024, Ideas on Board Oy
>> > + *
>> > + * timer-fail.cpp - Threaded timer failure test
>> > + */
>> > +
>> > +#include <chrono>
>> > +#include <iostream>
>> > +
>> > +#include <libcamera/base/event_dispatcher.h>
>> > +#include <libcamera/base/object.h>
>> > +#include <libcamera/base/thread.h>
>> > +#include <libcamera/base/timer.h>
>> > +
>> > +#include "test.h"
>> > +
>> > +using namespace libcamera;
>> > +using namespace std;
>> > +using namespace std::chrono_literals;
>> > +
>> > +class TimeoutHandler : public Object
>> > +{
>> > +public:
>> > +	TimeoutHandler()
>> > +		: timer_(this), timeout_(false)
>> > +	{
>> > +		timer_.timeout.connect(this, &TimeoutHandler::timeoutHandler);
>> > +	}
>> > +
>> > +	void start()
>> > +	{
>> > +		timer_.start(100ms);
>> > +	}
>> > +
>> > +	bool timeout() const
>> > +	{
>> > +		return timeout_;
>> > +	}
>> > +
>> > +private:
>> > +	void timeoutHandler()
>> > +	{
>> > +		timeout_ = true;
>> > +	}
>> > +
>> > +	Timer timer_;
>> > +	bool timeout_;
>> > +};
>> > +
>> > +class TimerFailTest : public Test
>> > +{
>> > +protected:
>> > +	int init()
>> > +	{
>> > +		thread_.start();
>> > +		timeout_.moveToThread(&thread_);
>> > +
>> > +		return TestPass;
>> > +	}
>> > +
>> > +	int run()
>> > +	{
>> > +		/*
>> > +		 * Test that the forbidden operation of starting the timer from
>> > +		 * another thread results in a failure. We need to interrupt the
>> > +		 * event dispatcher to make sure we don't succeed simply because
>> > +		 * the event dispatcher hasn't noticed the timer restart.
>> > +		 */
>> > +		timeout_.start();
>> > +		thread_.eventDispatcher()->interrupt();
>> > +
>> > +		this_thread::sleep_for(chrono::milliseconds(200));
>> > +
>> > +		/*
>> > +		 * Meson expects this test to fail, so we need to return
>> 
>> It'd be good to mention here that it's expected to fail on an assertion.
>
> I'll expand the comment to
>
> 		 * The wrong start() call should result in an assertion in debug
> 		 * builds, and a timeout in release builds. The test is
> 		 * therefore marked in meson.build as expected to fail. We need
> 		 * to return TestPass in the unexpected (usually known as
> 		 * "fail") case, and TestFail otherwise.

Sounds good, thanks.

>> > +		 * TestPass in the unexpected (usually known as "fail"), case,
>> > +		 * and TestFail otherwise.
>> > +		 */
>> > +		if (timeout_.timeout()) {
>> > +			cout << "Timer start from wrong thread succeeded unexpectedly"
>> > +			     << endl;
>> > +			return TestPass;
>> > +		}
>> > +
>> > +		return TestFail;
>> > +	}
>> > +
>> > +	void cleanup()
>> > +	{
>> > +		/* Must stop thread before destroying timeout. */
>> > +		thread_.exit(0);
>> > +		thread_.wait();
>> > +	}
>> > +
>> > +private:
>> > +	TimeoutHandler timeout_;
>> > +	Thread thread_;
>> > +};
>> > +
>> > +TEST_REGISTER(TimerFailTest)
>> > diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp
>> > index 0bcd0d8ce194..4caf4e33b2ba 100644
>> > --- a/test/timer-thread.cpp
>> > +++ b/test/timer-thread.cpp
>> > @@ -29,12 +29,6 @@ public:
>> >  		timer_.start(100ms);
>> >  	}
>> >  
>> > -	void restart()
>> > -	{
>> > -		timeout_ = false;
>> > -		timer_.start(100ms);
>> > -	}
>> > -
>> >  	bool timeout() const
>> >  	{
>> >  		return timeout_;
>> > @@ -74,22 +68,6 @@ protected:
>> >  			return TestFail;
>> >  		}
>> >  
>> > -		/*
>> > -		 * Test that starting the timer from another thread fails. We
>> > -		 * need to interrupt the event dispatcher to make sure we don't
>> > -		 * succeed simply because the event dispatcher hasn't noticed
>> > -		 * the timer restart.
>> > -		 */
>> > -		timeout_.restart();
>> > -		thread_.eventDispatcher()->interrupt();
>> > -
>> > -		this_thread::sleep_for(chrono::milliseconds(200));
>> > -
>> > -		if (timeout_.timeout()) {
>> > -			cout << "Timer restart test failed" << endl;
>> > -			return TestFail;
>> > -		}
>> > -
>> >  		return TestPass;
>> >  	}