[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:

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

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