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

[Kieran Bingham]

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

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