[libcamera-devel] [PATCH v9 5/5] lc-compliance: Add list and filter parameters

[Jacopo Mondi]

On Mon, Jun 28, 2021 at 10:47:21AM -0300, Nícolas F. R. A. Prado wrote:
> Hi Jacopo,

Hello Nicolas,

>
> On Sat, Jun 26, 2021 at 10:22:39AM +0200, Jacopo Mondi wrote:
> > Hi Nicolas,
> >
> > On Fri, Jun 25, 2021 at 04:39:25PM -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).
> > >
> > > While at it, add to the help message that further googletest options can
> > > be passed through the environment.
> > >
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > Changes in v9:
> > > - Thanks to Jacopo:
> > >   - Fixed some style issues
> > >   - Added to --help that further Googletest options can be passed as environment
> > >     variables
> > > - Thanks to Paul:
> > >   - Fixed compiling error on clang
> > >
> > > Changes in v8:
> > > - Thanks to Kieran:
> > >   - Switched from malloc/free to new/delete in gtest parameter allocation
> > >
> > > Changes in v7:
> > > - Thanks to Niklas:
> > >   - Removed intermediary filter string variable in Harness::initGtestParameters()
> > >
> > > 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 | 76 ++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 73 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> > > index b01768b5fc0b..45ec70351f2c 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',
> > >  };
> > >
> > > @@ -76,12 +78,73 @@ static int initCamera(CameraManager *cm, OptionsParser::Options options)
> > >  	return 0;
> > >  }
> > >
> > > +static int initGtestParameters(char *arg0, OptionsParser::Options options)
> > > +{
> > > +	const std::map<std::string, std::string> gtestFlags = { { "list", "--gtest_list_tests" },
> > > +								{ "filter", "--gtest_filter" } };
> > > +
> > > +	int argc = 0;
> > > +	std::string filterParam;
> > > +
> > > +	/*
> > > +	 * +2 to have space for both the 0th argument that is needed but not
> > > +	 * used and the null at the end.
> > > +	 */
> > > +	char **argv = new char *[(gtestFlags.size() + 2)];
> > > +
> >
> > Additional empty line
>
> By this you mean I should add one more empty line or remove this one?
>

If you add one more there will be 2 empty lines :)
I mean that you can remove it as the check immediately follows the
variable assignement. But the assignement is also the variable
declaration in this case, which usually calls for an emtpy line
after /o\ Whatever, anything is fine, really :)

> >
> > > +	if (!argv)
> > > +		return -ENOMEM;
> > > +
> > > +	argv[0] = arg0;
> > > +	argc++;
> > > +
> > > +	if (options.isSet(OptList)) {
> > > +		argv[argc] = const_cast<char *>(gtestFlags.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
> > > +		 */
> > > +		filterParam = gtestFlags.at("filter") + "=" +
> > > +			      static_cast<const std::string &>(options[OptFilter]);
> > > +
> > > +		argv[argc] = const_cast<char *>(filterParam.c_str());
> > > +		argc++;
> > > +	}
> > > +
> > > +	argv[argc] = nullptr;
> > > +
> > > +	::testing::InitGoogleTest(&argc, argv);
> > > +
> > > +	delete[] argv;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int initGtest(char *arg0, OptionsParser::Options options)
> > > +{
> > > +	int ret = initGtestParameters(arg0, options);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > > +
> > > +	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");
> > >
> > > @@ -91,6 +154,8 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options)
> > >
> > >  	if (options->isSet(OptHelp)) {
> > >  		parser.usage();
> > > +		std::cerr << "Further options from Googletest can be passed as environment variables"
> > > +			  << std::endl;
> > >  		return -EINTR;
> > >  	}
> > >
> > > @@ -108,12 +173,17 @@ int main(int argc, char **argv)
> > >
> > >  	std::unique_ptr<CameraManager> cm = std::make_unique<CameraManager>();
> > >
> > > -	ret = initCamera(cm.get(), options);
> > > +	/* No need to initialize the camera if we'll just list tests */
> >
> > Won't you in this case stop the cm without starting it ?
>
> Indeed. This doesn't seem to cause trouble, but I guess I can also check if
> OptList is set when stopping the cm.

You know, I'm not sure.. stop() will call exit() and wait()
(pthread_exit() and pthread_wait()) on the cm loop, which in this case
it has not been started. I haven't been able to clearly find any
indication that it's a no-op in that case :)

Also, deleting the cm calls stop() so you can avoid calling it
expliticitly if you prefer to do so!

Either way
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>
> Thanks,
> Nícolas
>
> >
> > > +	if (!options.isSet(OptList)) {
> > > +		ret = initCamera(cm.get(), options);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	ret = initGtest(argv[0], options);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
> > > -
> > >  	ret = RUN_ALL_TESTS();
> > >
> > >  	cm->stop();
> > > --
> > > 2.32.0
> > >