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

[Nícolas F. R. A. Prado]

Hi Jacopo,

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

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