[libcamera-devel] [PATCH v6 5/5] lc-compliance: Add list and filter parameters
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Jun 10 19:48:03 CEST 2021
Hi Nícolas,
On 2021-06-09 09:30:40 -0300, Nícolas F. R. A. Prado wrote:
> Hi Niklas,
>
> On Wed, Jun 09, 2021 at 12:40:04AM +0200, Niklas Söderlund wrote:
> > Hi Nícolas,
> >
> > Thanks for your work.
> >
> > On 2021-06-07 15:15:06 -0300, Nícolas F. R. A. Prado wrote:
> > > Add a --list parameter that lists all current tests (by mapping to
> > > googletest's --gtest_list_tests).
> > >
> > > Add a --filter 'filterString' parameter that filters the tests to run
> > > (by mapping to googletest's --gtest_filter).
> > >
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > ---
> > > Changes in v6:
> > > - Thanks to Niklas:
> > > - Changed buildGtestParameters() into initGtestParameters() and removed the
> > > need for Harness::gtestArgc_ and Harness::gtestArgv_
> > >
> > > Changes in v5:
> > > - Thanks to Niklas:
> > > - Moved buildGtestParameters() inside run()
> > >
> > > No changes in v4
> > >
> > > src/lc-compliance/main.cpp | 69 +++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 65 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > index 27503372d0eb..e35238471ee6 100644
> > > --- a/src/lc-compliance/main.cpp
> > > +++ b/src/lc-compliance/main.cpp
> > > @@ -20,6 +20,8 @@ using namespace libcamera;
> > >
> > > enum {
> > > OptCamera = 'c',
> > > + OptList = 'l',
> > > + OptFilter = 'f',
> > > OptHelp = 'h',
> > > };
> > >
> > > @@ -45,13 +47,17 @@ public:
> > > ~Harness();
> > >
> > > int init();
> > > - int run(int argc, char **argv);
> > > + int run(char *arg0);
> > >
> > > private:
> > > + int initGtestParameters(char *arg0);
> > > void listCameras();
> > >
> > > OptionsParser::Options options_;
> > > std::shared_ptr<CameraManager> cm_;
> > > +
> > > + const std::map<std::string, std::string> gtestFlag_ = { { "list", "--gtest_list_tests" },
> > > + { "filter", "--gtest_filter" } };
> > > };
> > >
> > > Harness::Harness(const OptionsParser::Options &options)
> > > @@ -76,6 +82,12 @@ int Harness::init()
> > > return ret;
> > > }
> > >
> > > + /*
> > > + * No need to initialize the camera if we'll just list tests
> > > + */
> > > + if (options_.isSet(OptList))
> > > + return 0;
> > > +
> > > if (!options_.isSet(OptCamera)) {
> > > std::cout << "No camera specified, available cameras:" << std::endl;
> > > listCameras();
> > > @@ -97,9 +109,11 @@ int Harness::init()
> > > return 0;
> > > }
> > >
> > > -int Harness::run(int argc, char **argv)
> > > +int Harness::run(char *arg0)
> > > {
> > > - ::testing::InitGoogleTest(&argc, argv);
> > > + int ret = initGtestParameters(arg0);
> > > + if (ret)
> > > + return ret;
> > >
> > > testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > >
> > > @@ -112,12 +126,59 @@ void Harness::listCameras()
> > > std::cout << "- " << cam.get()->id() << std::endl;
> > > }
> > >
> > > +int Harness::initGtestParameters(char *arg0)
> > > +{
> > > + int argc = 0;
> > > + char **argv;
> > > + std::string filterParam;
> > > +
> > > + /*
> > > + * +2 to have space for both the 0th argument that is needed but not
> > > + * used and the null at the end.
> > > + */
> > > + argv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *));
> > > + if (!argv)
> > > + return -ENOMEM;
> > > +
> > > + argv[argc] = arg0;
> > > + argc++;
> > > +
> > > + if (options_.isSet(OptList)) {
> > > + argv[argc] = const_cast<char *>(gtestFlag_.at("list").c_str());
> > > + argc++;
> > > + }
> > > +
> > > + if (options_.isSet(OptFilter)) {
> > > + /*
> > > + * The filter flag needs to be passed as a single parameter, in
> > > + * the format --gtest_filter=filterStr
> > > + */
> > > + const std::string &filter = options_[OptFilter];
> >
> > I think 'filter' needs to be declared in the initGtestParameters() scope
> > as it's consumed by ::testing::InitGoogleTest(). With this fixed,
>
> Actually what is consumed in ::testing::InitGoogleTest() is filterParam, which
> indeed is in initGtestParameters()'s scope. This filter string here is used just
> to compose the entire string just below and move it to filterParam. I'm assuming
> when I do
You are correct.
>
> filterParam = gtestFlag_.at("filter") + "=" + filter;
>
> I'm copying the combined contents of all those strings to filterParam, so it
> should be safe to use in ::testing::InitGoogleTest(), but please tell me if this
> is not the case :).
>
> But maybe it would be better to just omit that local filter variable? A cast
> seems to be needed in that case:
>
> filterParam = gtestFlag_.at("filter") + "=" +
> (const std::string&) options_[OptFilter];
>
> What do you think?
At least it would avoid me making a similar error again while reading
the code :-) I think both solutions are ok.
>
> Thanks,
> Nícolas
>
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > Nice work!
> >
> > > + filterParam = gtestFlag_.at("filter") + "=" + filter;
> > > +
> > > + argv[argc] = const_cast<char *>(filterParam.c_str());
> > > + argc++;
> > > + }
> > > +
> > > + argv[argc] = 0;
> > > +
> > > + ::testing::InitGoogleTest(&argc, argv);
> > > +
> > > + free(argv);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> > > {
> > > OptionsParser parser;
> > > parser.addOption(OptCamera, OptionString,
> > > "Specify which camera to operate on, by id", "camera",
> > > ArgumentRequired, "camera");
> > > + parser.addOption(OptList, OptionNone, "List all tests and exit", "list");
> > > + parser.addOption(OptFilter, OptionString,
> > > + "Specify which tests to run", "filter",
> > > + ArgumentRequired, "filter");
> > > parser.addOption(OptHelp, OptionNone, "Display this help message",
> > > "help");
> > >
> > > @@ -147,5 +208,5 @@ int main(int argc, char **argv)
> > > if (ret)
> > > return ret;
> > >
> > > - return harness.run(argc, argv);
> > > + return harness.run(argv[0]);
> > > }
> > > --
> > > 2.31.1
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list