[libcamera-devel,4/9] test: timer: Test that a timer can be restarted before it expires
diff mbox series

Message ID 20191006053226.8976-5-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Timer fixes and enhancements
Related show

Commit Message

Laurent Pinchart Oct. 6, 2019, 5:32 a.m. UTC 
The Timer API allows restarting a timer before it expires. Add a
corresponding test. The test fails as the Timer class doesn't comply
with its API documentation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 test/timer.cpp | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Jacopo Mondi Oct. 6, 2019, 6:28 p.m. UTC | #1 
Hi Laurent

On Sun, Oct 06, 2019 at 08:32:21AM +0300, Laurent Pinchart wrote:
> The Timer API allows restarting a timer before it expires. Add a
> corresponding test. The test fails as the Timer class doesn't comply
> with its API documentation.
>
[5/9] has subject:
libcamera: timer: Allow restarting a timer before expiration

Shouldn't this be moved before this test?

Thanks
  j

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  test/timer.cpp | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/test/timer.cpp b/test/timer.cpp
> index d4f16a9bdd97..5ff94dbbdeb0 100644
> --- a/test/timer.cpp
> +++ b/test/timer.cpp
> @@ -21,13 +21,14 @@ class ManagedTimer : public Timer
>  {
>  public:
>  	ManagedTimer()
> -		: Timer()
> +		: Timer(), count_(0)
>  	{
>  		timeout.connect(this, &ManagedTimer::timeoutHandler);
>  	}
>
>  	void start(int msec)
>  	{
> +		count_ = 0;
>  		start_ = std::chrono::steady_clock::now();
>  		expiration_ = std::chrono::steady_clock::time_point();
>
> @@ -40,12 +41,19 @@ public:
>  		return abs(std::chrono::duration_cast<std::chrono::milliseconds>(duration).count());
>  	}
>
> +	bool hasFailed()
> +	{
> +		return isRunning() || count_ != 1 || jitter() > 50;
> +	}
> +
>  private:
>  	void timeoutHandler(Timer *timer)
>  	{
>  		expiration_ = std::chrono::steady_clock::now();
> +		count_++;
>  	}
>
> +	unsigned int count_;
>  	std::chrono::steady_clock::time_point start_;
>  	std::chrono::steady_clock::time_point expiration_;
>  };
> @@ -74,7 +82,7 @@ protected:
>
>  		dispatcher->processEvents();
>
> -		if (timer.isRunning() || timer.jitter() > 50) {
> +		if (timer.hasFailed()) {
>  			cout << "Timer expiration test failed" << endl;
>  			return TestFail;
>  		}
> @@ -87,7 +95,7 @@ protected:
>  		timer.start(4295);
>  		dispatcher->processEvents();
>
> -		if (timer.isRunning() || timer.jitter() > 50) {
> +		if (timer.hasFailed()) {
>  			cout << "Timer expiration test failed" << endl;
>  			return TestFail;
>  		}
> @@ -102,11 +110,23 @@ protected:
>
>  		dispatcher->processEvents();
>
> -		if (timer.isRunning() || timer.jitter() > 50) {
> +		if (timer.hasFailed()) {
>  			cout << "Timer restart test failed" << endl;
>  			return TestFail;
>  		}
>
> +		/* Timer restart before expiration. */
> +		timer.start(50);
> +		timer.start(100);
> +		timer.start(150);
> +
> +		dispatcher->processEvents();
> +
> +		if (timer.hasFailed()) {
> +			cout << "Timer restart before expiration test failed" << endl;
> +			return TestFail;
> +		}
> +
>  		/* Two timers. */
>  		timer.start(1000);
>  		timer2.start(300);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 7, 2019, 2:59 a.m. UTC | #2 
Hi Jacopo,

On Sun, Oct 06, 2019 at 08:28:21PM +0200, Jacopo Mondi wrote:
> On Sun, Oct 06, 2019 at 08:32:21AM +0300, Laurent Pinchart wrote:
> > The Timer API allows restarting a timer before it expires. Add a
> > corresponding test. The test fails as the Timer class doesn't comply
> > with its API documentation.
> >
> [5/9] has subject:
> libcamera: timer: Allow restarting a timer before expiration
> 
> Shouldn't this be moved before this test?

When fixing a broken behaviour, I like to first have a test that fails
to exhibit the problem, and a fix on top. This allows proving that the
behaviour was broken (the test fails) and that the fix fixes it (the
test then succeeds).

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  test/timer.cpp | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/test/timer.cpp b/test/timer.cpp
> > index d4f16a9bdd97..5ff94dbbdeb0 100644
> > --- a/test/timer.cpp
> > +++ b/test/timer.cpp
> > @@ -21,13 +21,14 @@ class ManagedTimer : public Timer
> >  {
> >  public:
> >  	ManagedTimer()
> > -		: Timer()
> > +		: Timer(), count_(0)
> >  	{
> >  		timeout.connect(this, &ManagedTimer::timeoutHandler);
> >  	}
> >
> >  	void start(int msec)
> >  	{
> > +		count_ = 0;
> >  		start_ = std::chrono::steady_clock::now();
> >  		expiration_ = std::chrono::steady_clock::time_point();
> >
> > @@ -40,12 +41,19 @@ public:
> >  		return abs(std::chrono::duration_cast<std::chrono::milliseconds>(duration).count());
> >  	}
> >
> > +	bool hasFailed()
> > +	{
> > +		return isRunning() || count_ != 1 || jitter() > 50;
> > +	}
> > +
> >  private:
> >  	void timeoutHandler(Timer *timer)
> >  	{
> >  		expiration_ = std::chrono::steady_clock::now();
> > +		count_++;
> >  	}
> >
> > +	unsigned int count_;
> >  	std::chrono::steady_clock::time_point start_;
> >  	std::chrono::steady_clock::time_point expiration_;
> >  };
> > @@ -74,7 +82,7 @@ protected:
> >
> >  		dispatcher->processEvents();
> >
> > -		if (timer.isRunning() || timer.jitter() > 50) {
> > +		if (timer.hasFailed()) {
> >  			cout << "Timer expiration test failed" << endl;
> >  			return TestFail;
> >  		}
> > @@ -87,7 +95,7 @@ protected:
> >  		timer.start(4295);
> >  		dispatcher->processEvents();
> >
> > -		if (timer.isRunning() || timer.jitter() > 50) {
> > +		if (timer.hasFailed()) {
> >  			cout << "Timer expiration test failed" << endl;
> >  			return TestFail;
> >  		}
> > @@ -102,11 +110,23 @@ protected:
> >
> >  		dispatcher->processEvents();
> >
> > -		if (timer.isRunning() || timer.jitter() > 50) {
> > +		if (timer.hasFailed()) {
> >  			cout << "Timer restart test failed" << endl;
> >  			return TestFail;
> >  		}
> >
> > +		/* Timer restart before expiration. */
> > +		timer.start(50);
> > +		timer.start(100);
> > +		timer.start(150);
> > +
> > +		dispatcher->processEvents();
> > +
> > +		if (timer.hasFailed()) {
> > +			cout << "Timer restart before expiration test failed" << endl;
> > +			return TestFail;
> > +		}
> > +
> >  		/* Two timers. */
> >  		timer.start(1000);
> >  		timer2.start(300);

Patch
diff mbox series 

diff --git a/test/timer.cpp b/test/timer.cpp
index d4f16a9bdd97..5ff94dbbdeb0 100644
--- a/test/timer.cpp
+++ b/test/timer.cpp
@@ -21,13 +21,14 @@  class ManagedTimer : public Timer
 {
 public:
 	ManagedTimer()
-		: Timer()
+		: Timer(), count_(0)
 	{
 		timeout.connect(this, &ManagedTimer::timeoutHandler);
 	}
 
 	void start(int msec)
 	{
+		count_ = 0;
 		start_ = std::chrono::steady_clock::now();
 		expiration_ = std::chrono::steady_clock::time_point();
 
@@ -40,12 +41,19 @@  public:
 		return abs(std::chrono::duration_cast<std::chrono::milliseconds>(duration).count());
 	}
 
+	bool hasFailed()
+	{
+		return isRunning() || count_ != 1 || jitter() > 50;
+	}
+
 private:
 	void timeoutHandler(Timer *timer)
 	{
 		expiration_ = std::chrono::steady_clock::now();
+		count_++;
 	}
 
+	unsigned int count_;
 	std::chrono::steady_clock::time_point start_;
 	std::chrono::steady_clock::time_point expiration_;
 };
@@ -74,7 +82,7 @@  protected:
 
 		dispatcher->processEvents();
 
-		if (timer.isRunning() || timer.jitter() > 50) {
+		if (timer.hasFailed()) {
 			cout << "Timer expiration test failed" << endl;
 			return TestFail;
 		}
@@ -87,7 +95,7 @@  protected:
 		timer.start(4295);
 		dispatcher->processEvents();
 
-		if (timer.isRunning() || timer.jitter() > 50) {
+		if (timer.hasFailed()) {
 			cout << "Timer expiration test failed" << endl;
 			return TestFail;
 		}
@@ -102,11 +110,23 @@  protected:
 
 		dispatcher->processEvents();
 
-		if (timer.isRunning() || timer.jitter() > 50) {
+		if (timer.hasFailed()) {
 			cout << "Timer restart test failed" << endl;
 			return TestFail;
 		}
 
+		/* Timer restart before expiration. */
+		timer.start(50);
+		timer.start(100);
+		timer.start(150);
+
+		dispatcher->processEvents();
+
+		if (timer.hasFailed()) {
+			cout << "Timer restart before expiration test failed" << endl;
+			return TestFail;
+		}
+
 		/* Two timers. */
 		timer.start(1000);
 		timer2.start(300);