[libcamera-devel,v1,1/2] test: Store path to the test executable in Test class
diff mbox series

Message ID 20211130012303.10574-2-laurent.pinchart@ideasonboard.com
State Accepted
Commit b24d9c4413b9d7f55f4105adeb7cf9a2450d3204
Headers show
Series
  • libcamera: Fix valgrind usage with tests that fork process
Related show

Commit Message

Laurent Pinchart Nov. 30, 2021, 1:23 a.m. UTC
Store the path to the test executable, found in argv[0], in the Test
instance. This can be useful for tests that need to fork processes.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 test/ipc/unixsocket.cpp       |  4 +++-
 test/ipc/unixsocket_ipc.cpp   |  4 +++-
 test/libtest/test.cpp         |  5 +++++
 test/libtest/test.h           | 15 ++++++++++++---
 test/log/log_process.cpp      |  4 +++-
 test/process/process_test.cpp |  5 +++--
 6 files changed, 29 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi Nov. 30, 2021, 8:24 a.m. UTC | #1
Hi Laurent

On Tue, Nov 30, 2021 at 03:23:02AM +0200, Laurent Pinchart wrote:
> Store the path to the test executable, found in argv[0], in the Test
> instance. This can be useful for tests that need to fork processes.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  test/ipc/unixsocket.cpp       |  4 +++-
>  test/ipc/unixsocket_ipc.cpp   |  4 +++-
>  test/libtest/test.cpp         |  5 +++++
>  test/libtest/test.h           | 15 ++++++++++++---
>  test/log/log_process.cpp      |  4 +++-
>  test/process/process_test.cpp |  5 +++--
>  6 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> index 7270bf4d2fe7..4fc1c10a2125 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -501,5 +501,7 @@ int main(int argc, char **argv)
>  		return slave.run(ipcfd);
>  	}
>
> -	return UnixSocketTest().execute();
> +	UnixSocketTest test;
> +	test.setArgs(argc, argv);
> +	return test.execute();
>  }
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> index ab5d25572d83..2e3b52ca4d4b 100644
> --- a/test/ipc/unixsocket_ipc.cpp
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -227,5 +227,7 @@ int main(int argc, char **argv)
>  		return slave.run(ipcfd);
>  	}
>
> -	return UnixSocketTestIPC().execute();
> +	UnixSocketTestIPC test;
> +	test.setArgs(argc, argv);
> +	return test.execute();
>  }
> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> index fd9f3d74d6ce..af37b4dd28ff 100644
> --- a/test/libtest/test.cpp
> +++ b/test/libtest/test.cpp
> @@ -17,6 +17,11 @@ Test::~Test()
>  {
>  }
>
> +void Test::setArgs([[maybe_unused]] int argc, char *argv[])
> +{
> +	self_ = argv[0];

Paranoia says this could be made intentionally very very long

Apart from that, for the series

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +}
> +
>  int Test::execute()
>  {
>  	int ret;
> diff --git a/test/libtest/test.h b/test/libtest/test.h
> index ee01a22591f8..23b07743fd2a 100644
> --- a/test/libtest/test.h
> +++ b/test/libtest/test.h
> @@ -8,6 +8,7 @@
>  #pragma once
>
>  #include <sstream>
> +#include <string>
>
>  enum TestStatus {
>  	TestPass = 0,
> @@ -21,16 +22,24 @@ public:
>  	Test();
>  	virtual ~Test();
>
> +	void setArgs(int argc, char *argv[]);
>  	int execute();
>
> +	const std::string &self() const { return self_; }
> +
>  protected:
>  	virtual int init() { return 0; }
>  	virtual int run() = 0;
>  	virtual void cleanup() {}
> +
> +private:
> +	std::string self_;
>  };
>
> -#define TEST_REGISTER(klass)						\
> -int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])	\
> +#define TEST_REGISTER(Klass)						\
> +int main(int argc, char *argv[])					\
>  {									\
> -	return klass().execute();					\
> +	Klass klass;							\
> +	klass.setArgs(argc, argv);					\
> +	return klass.execute();						\
>  }
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index a56a399848a7..ca8351335f3a 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -154,5 +154,7 @@ int main(int argc, char **argv)
>  		return child.run(status, num);
>  	}
>
> -	return LogProcessTest().execute();
> +	LogProcessTest test;
> +	test.setArgs(argc, argv);
> +	return test.execute();
>  }
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index 378d680bf4ef..96bea17f8dce 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -9,7 +9,6 @@
>  #include <unistd.h>
>  #include <vector>
>
> -
>  #include <libcamera/base/event_dispatcher.h>
>  #include <libcamera/base/thread.h>
>  #include <libcamera/base/timer.h>
> @@ -106,5 +105,7 @@ int main(int argc, char **argv)
>  		return child.run(status);
>  	}
>
> -	return ProcessTest().execute();
> +	ProcessTest test;
> +	test.setArgs(argc, argv);
> +	return test.execute();
>  }
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Nov. 30, 2021, 9:21 a.m. UTC | #2
Hi Jacopo,

On Tue, Nov 30, 2021 at 09:24:14AM +0100, Jacopo Mondi wrote:
> On Tue, Nov 30, 2021 at 03:23:02AM +0200, Laurent Pinchart wrote:
> > Store the path to the test executable, found in argv[0], in the Test
> > instance. This can be useful for tests that need to fork processes.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  test/ipc/unixsocket.cpp       |  4 +++-
> >  test/ipc/unixsocket_ipc.cpp   |  4 +++-
> >  test/libtest/test.cpp         |  5 +++++
> >  test/libtest/test.h           | 15 ++++++++++++---
> >  test/log/log_process.cpp      |  4 +++-
> >  test/process/process_test.cpp |  5 +++--
> >  6 files changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> > index 7270bf4d2fe7..4fc1c10a2125 100644
> > --- a/test/ipc/unixsocket.cpp
> > +++ b/test/ipc/unixsocket.cpp
> > @@ -501,5 +501,7 @@ int main(int argc, char **argv)
> >  		return slave.run(ipcfd);
> >  	}
> >
> > -	return UnixSocketTest().execute();
> > +	UnixSocketTest test;
> > +	test.setArgs(argc, argv);
> > +	return test.execute();
> >  }
> > diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> > index ab5d25572d83..2e3b52ca4d4b 100644
> > --- a/test/ipc/unixsocket_ipc.cpp
> > +++ b/test/ipc/unixsocket_ipc.cpp
> > @@ -227,5 +227,7 @@ int main(int argc, char **argv)
> >  		return slave.run(ipcfd);
> >  	}
> >
> > -	return UnixSocketTestIPC().execute();
> > +	UnixSocketTestIPC test;
> > +	test.setArgs(argc, argv);
> > +	return test.execute();
> >  }
> > diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> > index fd9f3d74d6ce..af37b4dd28ff 100644
> > --- a/test/libtest/test.cpp
> > +++ b/test/libtest/test.cpp
> > @@ -17,6 +17,11 @@ Test::~Test()
> >  {
> >  }
> >
> > +void Test::setArgs([[maybe_unused]] int argc, char *argv[])
> > +{
> > +	self_ = argv[0];
> 
> Paranoia says this could be made intentionally very very long

Limited to PATH_MAX probably (which should be 4096 bytes at most on most
systems I think). And if someone wants to attack unit tests on their own
system, well, I won't stop them :-)

> Apart from that, for the series
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > +}
> > +
> >  int Test::execute()
> >  {
> >  	int ret;
> > diff --git a/test/libtest/test.h b/test/libtest/test.h
> > index ee01a22591f8..23b07743fd2a 100644
> > --- a/test/libtest/test.h
> > +++ b/test/libtest/test.h
> > @@ -8,6 +8,7 @@
> >  #pragma once
> >
> >  #include <sstream>
> > +#include <string>
> >
> >  enum TestStatus {
> >  	TestPass = 0,
> > @@ -21,16 +22,24 @@ public:
> >  	Test();
> >  	virtual ~Test();
> >
> > +	void setArgs(int argc, char *argv[]);
> >  	int execute();
> >
> > +	const std::string &self() const { return self_; }
> > +
> >  protected:
> >  	virtual int init() { return 0; }
> >  	virtual int run() = 0;
> >  	virtual void cleanup() {}
> > +
> > +private:
> > +	std::string self_;
> >  };
> >
> > -#define TEST_REGISTER(klass)						\
> > -int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])	\
> > +#define TEST_REGISTER(Klass)						\
> > +int main(int argc, char *argv[])					\
> >  {									\
> > -	return klass().execute();					\
> > +	Klass klass;							\
> > +	klass.setArgs(argc, argv);					\
> > +	return klass.execute();						\
> >  }
> > diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> > index a56a399848a7..ca8351335f3a 100644
> > --- a/test/log/log_process.cpp
> > +++ b/test/log/log_process.cpp
> > @@ -154,5 +154,7 @@ int main(int argc, char **argv)
> >  		return child.run(status, num);
> >  	}
> >
> > -	return LogProcessTest().execute();
> > +	LogProcessTest test;
> > +	test.setArgs(argc, argv);
> > +	return test.execute();
> >  }
> > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> > index 378d680bf4ef..96bea17f8dce 100644
> > --- a/test/process/process_test.cpp
> > +++ b/test/process/process_test.cpp
> > @@ -9,7 +9,6 @@
> >  #include <unistd.h>
> >  #include <vector>
> >
> > -
> >  #include <libcamera/base/event_dispatcher.h>
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/timer.h>
> > @@ -106,5 +105,7 @@ int main(int argc, char **argv)
> >  		return child.run(status);
> >  	}
> >
> > -	return ProcessTest().execute();
> > +	ProcessTest test;
> > +	test.setArgs(argc, argv);
> > +	return test.execute();
> >  }
Kieran Bingham Nov. 30, 2021, 10:55 a.m. UTC | #3
Quoting Laurent Pinchart (2021-11-30 01:23:02)
> Store the path to the test executable, found in argv[0], in the Test
> instance. This can be useful for tests that need to fork processes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  test/ipc/unixsocket.cpp       |  4 +++-
>  test/ipc/unixsocket_ipc.cpp   |  4 +++-
>  test/libtest/test.cpp         |  5 +++++
>  test/libtest/test.h           | 15 ++++++++++++---
>  test/log/log_process.cpp      |  4 +++-
>  test/process/process_test.cpp |  5 +++--
>  6 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> index 7270bf4d2fe7..4fc1c10a2125 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -501,5 +501,7 @@ int main(int argc, char **argv)
>                 return slave.run(ipcfd);
>         }
>  
> -       return UnixSocketTest().execute();
> +       UnixSocketTest test;
> +       test.setArgs(argc, argv);
> +       return test.execute();
>  }
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> index ab5d25572d83..2e3b52ca4d4b 100644
> --- a/test/ipc/unixsocket_ipc.cpp
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -227,5 +227,7 @@ int main(int argc, char **argv)
>                 return slave.run(ipcfd);
>         }
>  
> -       return UnixSocketTestIPC().execute();
> +       UnixSocketTestIPC test;
> +       test.setArgs(argc, argv);
> +       return test.execute();
>  }
> diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> index fd9f3d74d6ce..af37b4dd28ff 100644
> --- a/test/libtest/test.cpp
> +++ b/test/libtest/test.cpp
> @@ -17,6 +17,11 @@ Test::~Test()
>  {
>  }
>  
> +void Test::setArgs([[maybe_unused]] int argc, char *argv[])
> +{
> +       self_ = argv[0];
> +}
> +
>  int Test::execute()
>  {
>         int ret;
> diff --git a/test/libtest/test.h b/test/libtest/test.h
> index ee01a22591f8..23b07743fd2a 100644
> --- a/test/libtest/test.h
> +++ b/test/libtest/test.h
> @@ -8,6 +8,7 @@
>  #pragma once
>  
>  #include <sstream>
> +#include <string>
>  
>  enum TestStatus {
>         TestPass = 0,
> @@ -21,16 +22,24 @@ public:
>         Test();
>         virtual ~Test();
>  
> +       void setArgs(int argc, char *argv[]);

Hrm... I would have been tempted to pass these into the constructor.

They are now somewhat arbitrarily 'required' in some tests, so it may as
well be that they are always required. (or at least requierd to be
considered).

>         int execute();
>  
> +       const std::string &self() const { return self_; }
> +
>  protected:
>         virtual int init() { return 0; }
>         virtual int run() = 0;
>         virtual void cleanup() {}
> +
> +private:
> +       std::string self_;
>  };
>  
> -#define TEST_REGISTER(klass)                                           \
> -int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])     \
> +#define TEST_REGISTER(Klass)                                           \
> +int main(int argc, char *argv[])                                       \
>  {                                                                      \
> -       return klass().execute();                                       \
> +       Klass klass;                                                    \
> +       klass.setArgs(argc, argv);                                      \
> +       return klass.execute();                                         \
>  }
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index a56a399848a7..ca8351335f3a 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -154,5 +154,7 @@ int main(int argc, char **argv)
>                 return child.run(status, num);
>         }
>  
> -       return LogProcessTest().execute();
> +       LogProcessTest test;
> +       test.setArgs(argc, argv);

Especially as here we see that now we have to manually 'know' that we
have to make a call to setArgs() after every construction...?

It's local test code, so it's not particularly critical though.

So if you want to keep it this way so-be-it.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +       return test.execute();
>  }
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index 378d680bf4ef..96bea17f8dce 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -9,7 +9,6 @@
>  #include <unistd.h>
>  #include <vector>
>  
> -
>  #include <libcamera/base/event_dispatcher.h>
>  #include <libcamera/base/thread.h>
>  #include <libcamera/base/timer.h>
> @@ -106,5 +105,7 @@ int main(int argc, char **argv)
>                 return child.run(status);
>         }
>  
> -       return ProcessTest().execute();
> +       ProcessTest test;
> +       test.setArgs(argc, argv);
> +       return test.execute();
>  }
> -- 
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Nov. 30, 2021, 11:02 a.m. UTC | #4
Hi Kieran,

On Tue, Nov 30, 2021 at 10:55:42AM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-11-30 01:23:02)
> > Store the path to the test executable, found in argv[0], in the Test
> > instance. This can be useful for tests that need to fork processes.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  test/ipc/unixsocket.cpp       |  4 +++-
> >  test/ipc/unixsocket_ipc.cpp   |  4 +++-
> >  test/libtest/test.cpp         |  5 +++++
> >  test/libtest/test.h           | 15 ++++++++++++---
> >  test/log/log_process.cpp      |  4 +++-
> >  test/process/process_test.cpp |  5 +++--
> >  6 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> > index 7270bf4d2fe7..4fc1c10a2125 100644
> > --- a/test/ipc/unixsocket.cpp
> > +++ b/test/ipc/unixsocket.cpp
> > @@ -501,5 +501,7 @@ int main(int argc, char **argv)
> >                 return slave.run(ipcfd);
> >         }
> >  
> > -       return UnixSocketTest().execute();
> > +       UnixSocketTest test;
> > +       test.setArgs(argc, argv);
> > +       return test.execute();
> >  }
> > diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> > index ab5d25572d83..2e3b52ca4d4b 100644
> > --- a/test/ipc/unixsocket_ipc.cpp
> > +++ b/test/ipc/unixsocket_ipc.cpp
> > @@ -227,5 +227,7 @@ int main(int argc, char **argv)
> >                 return slave.run(ipcfd);
> >         }
> >  
> > -       return UnixSocketTestIPC().execute();
> > +       UnixSocketTestIPC test;
> > +       test.setArgs(argc, argv);
> > +       return test.execute();
> >  }
> > diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> > index fd9f3d74d6ce..af37b4dd28ff 100644
> > --- a/test/libtest/test.cpp
> > +++ b/test/libtest/test.cpp
> > @@ -17,6 +17,11 @@ Test::~Test()
> >  {
> >  }
> >  
> > +void Test::setArgs([[maybe_unused]] int argc, char *argv[])
> > +{
> > +       self_ = argv[0];
> > +}
> > +
> >  int Test::execute()
> >  {
> >         int ret;
> > diff --git a/test/libtest/test.h b/test/libtest/test.h
> > index ee01a22591f8..23b07743fd2a 100644
> > --- a/test/libtest/test.h
> > +++ b/test/libtest/test.h
> > @@ -8,6 +8,7 @@
> >  #pragma once
> >  
> >  #include <sstream>
> > +#include <string>
> >  
> >  enum TestStatus {
> >         TestPass = 0,
> > @@ -21,16 +22,24 @@ public:
> >         Test();
> >         virtual ~Test();
> >  
> > +       void setArgs(int argc, char *argv[]);
> 
> Hrm... I would have been tempted to pass these into the constructor.
> 
> They are now somewhat arbitrarily 'required' in some tests, so it may as
> well be that they are always required. (or at least requierd to be
> considered).

That's what I initially did, but then every single test will have to
define a constructor just to forward the arguments to the base Test
class. I thought I'd get some push back :-) If you think that's better,
I'll go that way.

> >         int execute();
> >  
> > +       const std::string &self() const { return self_; }
> > +
> >  protected:
> >         virtual int init() { return 0; }
> >         virtual int run() = 0;
> >         virtual void cleanup() {}
> > +
> > +private:
> > +       std::string self_;
> >  };
> >  
> > -#define TEST_REGISTER(klass)                                           \
> > -int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])     \
> > +#define TEST_REGISTER(Klass)                                           \
> > +int main(int argc, char *argv[])                                       \
> >  {                                                                      \
> > -       return klass().execute();                                       \
> > +       Klass klass;                                                    \
> > +       klass.setArgs(argc, argv);                                      \
> > +       return klass.execute();                                         \
> >  }
> > diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> > index a56a399848a7..ca8351335f3a 100644
> > --- a/test/log/log_process.cpp
> > +++ b/test/log/log_process.cpp
> > @@ -154,5 +154,7 @@ int main(int argc, char **argv)
> >                 return child.run(status, num);
> >         }
> >  
> > -       return LogProcessTest().execute();
> > +       LogProcessTest test;
> > +       test.setArgs(argc, argv);
> 
> Especially as here we see that now we have to manually 'know' that we
> have to make a call to setArgs() after every construction...?
> 
> It's local test code, so it's not particularly critical though.
> 
> So if you want to keep it this way so-be-it.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> > +       return test.execute();
> >  }
> > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> > index 378d680bf4ef..96bea17f8dce 100644
> > --- a/test/process/process_test.cpp
> > +++ b/test/process/process_test.cpp
> > @@ -9,7 +9,6 @@
> >  #include <unistd.h>
> >  #include <vector>
> >  
> > -
> >  #include <libcamera/base/event_dispatcher.h>
> >  #include <libcamera/base/thread.h>
> >  #include <libcamera/base/timer.h>
> > @@ -106,5 +105,7 @@ int main(int argc, char **argv)
> >                 return child.run(status);
> >         }
> >  
> > -       return ProcessTest().execute();
> > +       ProcessTest test;
> > +       test.setArgs(argc, argv);
> > +       return test.execute();
> >  }
Kieran Bingham Nov. 30, 2021, 12:28 p.m. UTC | #5
Quoting Laurent Pinchart (2021-11-30 11:02:33)
> Hi Kieran,
> 
> On Tue, Nov 30, 2021 at 10:55:42AM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2021-11-30 01:23:02)
> > > Store the path to the test executable, found in argv[0], in the Test
> > > instance. This can be useful for tests that need to fork processes.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  test/ipc/unixsocket.cpp       |  4 +++-
> > >  test/ipc/unixsocket_ipc.cpp   |  4 +++-
> > >  test/libtest/test.cpp         |  5 +++++
> > >  test/libtest/test.h           | 15 ++++++++++++---
> > >  test/log/log_process.cpp      |  4 +++-
> > >  test/process/process_test.cpp |  5 +++--
> > >  6 files changed, 29 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> > > index 7270bf4d2fe7..4fc1c10a2125 100644
> > > --- a/test/ipc/unixsocket.cpp
> > > +++ b/test/ipc/unixsocket.cpp
> > > @@ -501,5 +501,7 @@ int main(int argc, char **argv)
> > >                 return slave.run(ipcfd);
> > >         }
> > >  
> > > -       return UnixSocketTest().execute();
> > > +       UnixSocketTest test;
> > > +       test.setArgs(argc, argv);
> > > +       return test.execute();
> > >  }
> > > diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> > > index ab5d25572d83..2e3b52ca4d4b 100644
> > > --- a/test/ipc/unixsocket_ipc.cpp
> > > +++ b/test/ipc/unixsocket_ipc.cpp
> > > @@ -227,5 +227,7 @@ int main(int argc, char **argv)
> > >                 return slave.run(ipcfd);
> > >         }
> > >  
> > > -       return UnixSocketTestIPC().execute();
> > > +       UnixSocketTestIPC test;
> > > +       test.setArgs(argc, argv);
> > > +       return test.execute();
> > >  }
> > > diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
> > > index fd9f3d74d6ce..af37b4dd28ff 100644
> > > --- a/test/libtest/test.cpp
> > > +++ b/test/libtest/test.cpp
> > > @@ -17,6 +17,11 @@ Test::~Test()
> > >  {
> > >  }
> > >  
> > > +void Test::setArgs([[maybe_unused]] int argc, char *argv[])
> > > +{
> > > +       self_ = argv[0];
> > > +}
> > > +
> > >  int Test::execute()
> > >  {
> > >         int ret;
> > > diff --git a/test/libtest/test.h b/test/libtest/test.h
> > > index ee01a22591f8..23b07743fd2a 100644
> > > --- a/test/libtest/test.h
> > > +++ b/test/libtest/test.h
> > > @@ -8,6 +8,7 @@
> > >  #pragma once
> > >  
> > >  #include <sstream>
> > > +#include <string>
> > >  
> > >  enum TestStatus {
> > >         TestPass = 0,
> > > @@ -21,16 +22,24 @@ public:
> > >         Test();
> > >         virtual ~Test();
> > >  
> > > +       void setArgs(int argc, char *argv[]);
> > 
> > Hrm... I would have been tempted to pass these into the constructor.
> > 
> > They are now somewhat arbitrarily 'required' in some tests, so it may as
> > well be that they are always required. (or at least requierd to be
> > considered).
> 
> That's what I initially did, but then every single test will have to
> define a constructor just to forward the arguments to the base Test
> class. I thought I'd get some push back :-) If you think that's better,
> I'll go that way.

Argh, I had forgotten/not realised that the Test() class is subclassed
to make the tests, so indeed then everything has to have a constructor
defined explicitly.

Yes, I can see the pain there...

Ok - probably not much better that way, so lets keep it like this. Not
fond of either ;-) so stick with the least effort...

--
Kieran


> 
> > >         int execute();
> > >  
> > > +       const std::string &self() const { return self_; }
> > > +
> > >  protected:
> > >         virtual int init() { return 0; }
> > >         virtual int run() = 0;
> > >         virtual void cleanup() {}
> > > +
> > > +private:
> > > +       std::string self_;
> > >  };
> > >  
> > > -#define TEST_REGISTER(klass)                                           \
> > > -int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])     \
> > > +#define TEST_REGISTER(Klass)                                           \
> > > +int main(int argc, char *argv[])                                       \
> > >  {                                                                      \
> > > -       return klass().execute();                                       \
> > > +       Klass klass;                                                    \
> > > +       klass.setArgs(argc, argv);                                      \
> > > +       return klass.execute();                                         \
> > >  }
> > > diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> > > index a56a399848a7..ca8351335f3a 100644
> > > --- a/test/log/log_process.cpp
> > > +++ b/test/log/log_process.cpp
> > > @@ -154,5 +154,7 @@ int main(int argc, char **argv)
> > >                 return child.run(status, num);
> > >         }
> > >  
> > > -       return LogProcessTest().execute();
> > > +       LogProcessTest test;
> > > +       test.setArgs(argc, argv);
> > 
> > Especially as here we see that now we have to manually 'know' that we
> > have to make a call to setArgs() after every construction...?
> > 
> > It's local test code, so it's not particularly critical though.
> > 
> > So if you want to keep it this way so-be-it.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > 
> > > +       return test.execute();
> > >  }
> > > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> > > index 378d680bf4ef..96bea17f8dce 100644
> > > --- a/test/process/process_test.cpp
> > > +++ b/test/process/process_test.cpp
> > > @@ -9,7 +9,6 @@
> > >  #include <unistd.h>
> > >  #include <vector>
> > >  
> > > -
> > >  #include <libcamera/base/event_dispatcher.h>
> > >  #include <libcamera/base/thread.h>
> > >  #include <libcamera/base/timer.h>
> > > @@ -106,5 +105,7 @@ int main(int argc, char **argv)
> > >                 return child.run(status);
> > >         }
> > >  
> > > -       return ProcessTest().execute();
> > > +       ProcessTest test;
> > > +       test.setArgs(argc, argv);
> > > +       return test.execute();
> > >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
index 7270bf4d2fe7..4fc1c10a2125 100644
--- a/test/ipc/unixsocket.cpp
+++ b/test/ipc/unixsocket.cpp
@@ -501,5 +501,7 @@  int main(int argc, char **argv)
 		return slave.run(ipcfd);
 	}
 
-	return UnixSocketTest().execute();
+	UnixSocketTest test;
+	test.setArgs(argc, argv);
+	return test.execute();
 }
diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
index ab5d25572d83..2e3b52ca4d4b 100644
--- a/test/ipc/unixsocket_ipc.cpp
+++ b/test/ipc/unixsocket_ipc.cpp
@@ -227,5 +227,7 @@  int main(int argc, char **argv)
 		return slave.run(ipcfd);
 	}
 
-	return UnixSocketTestIPC().execute();
+	UnixSocketTestIPC test;
+	test.setArgs(argc, argv);
+	return test.execute();
 }
diff --git a/test/libtest/test.cpp b/test/libtest/test.cpp
index fd9f3d74d6ce..af37b4dd28ff 100644
--- a/test/libtest/test.cpp
+++ b/test/libtest/test.cpp
@@ -17,6 +17,11 @@  Test::~Test()
 {
 }
 
+void Test::setArgs([[maybe_unused]] int argc, char *argv[])
+{
+	self_ = argv[0];
+}
+
 int Test::execute()
 {
 	int ret;
diff --git a/test/libtest/test.h b/test/libtest/test.h
index ee01a22591f8..23b07743fd2a 100644
--- a/test/libtest/test.h
+++ b/test/libtest/test.h
@@ -8,6 +8,7 @@ 
 #pragma once
 
 #include <sstream>
+#include <string>
 
 enum TestStatus {
 	TestPass = 0,
@@ -21,16 +22,24 @@  public:
 	Test();
 	virtual ~Test();
 
+	void setArgs(int argc, char *argv[]);
 	int execute();
 
+	const std::string &self() const { return self_; }
+
 protected:
 	virtual int init() { return 0; }
 	virtual int run() = 0;
 	virtual void cleanup() {}
+
+private:
+	std::string self_;
 };
 
-#define TEST_REGISTER(klass)						\
-int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[])	\
+#define TEST_REGISTER(Klass)						\
+int main(int argc, char *argv[])					\
 {									\
-	return klass().execute();					\
+	Klass klass;							\
+	klass.setArgs(argc, argv);					\
+	return klass.execute();						\
 }
diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
index a56a399848a7..ca8351335f3a 100644
--- a/test/log/log_process.cpp
+++ b/test/log/log_process.cpp
@@ -154,5 +154,7 @@  int main(int argc, char **argv)
 		return child.run(status, num);
 	}
 
-	return LogProcessTest().execute();
+	LogProcessTest test;
+	test.setArgs(argc, argv);
+	return test.execute();
 }
diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
index 378d680bf4ef..96bea17f8dce 100644
--- a/test/process/process_test.cpp
+++ b/test/process/process_test.cpp
@@ -9,7 +9,6 @@ 
 #include <unistd.h>
 #include <vector>
 
-
 #include <libcamera/base/event_dispatcher.h>
 #include <libcamera/base/thread.h>
 #include <libcamera/base/timer.h>
@@ -106,5 +105,7 @@  int main(int argc, char **argv)
 		return child.run(status);
 	}
 
-	return ProcessTest().execute();
+	ProcessTest test;
+	test.setArgs(argc, argv);
+	return test.execute();
 }