[libcamera-devel] [PATCH v1 1/2] test: Store path to the test executable in Test class

[Laurent Pinchart]

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