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

19430 diff mbox series

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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Laurent Pinchart <laurent.pinchart@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?

> Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

> +		 * 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;
>  	}

On Mon, Jan 22, 2024 at 09:17:56PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@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.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

> > +		 * 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;
> >  	}