[libcamera-devel] [PATCH v3 4/4] lc-compliance: Add list and filter parameters

[Niklas Söderlund]

Hi Nícolas,

Thanks for your work.

On 2021-05-14 10:16:52 -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>
> ---
>  src/lc-compliance/main.cpp | 68 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index d5ff93f514df..bebf1c51f64b 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',
>  };
>  
> @@ -54,16 +56,26 @@ public:
>  	~Harness();
>  
>  	int init();
> +	int buildGtestParameters(char *arg0);
> +	int gtestArgc() { return gtestArgc_; };
> +	char **gtestArgv() { return gtestArgv_; };

This feels a bit like a layer violation.

Do you think it would make sens to instead move the gtest data 
structures inside the Harness class? Or perhaps the Harness class is not 
as useful now that it likely won't grow to support all the test 
infrastructure provided by gtest and should instead be removed and what 
is left of it moved to main() and such helpers?

>  
>  private:
>  	void listCameras();
>  
>  	OptionsParser::Options options_;
>  	std::unique_ptr<CameraManager> cm_;
> +
> +	const std::map<std::string, std::string> gtestFlag_ = {{"list", "--gtest_list_tests"},
> +							       {"filter", "--gtest_filter"}};
> +
> +	int gtestArgc_;
> +	char **gtestArgv_;
> +	std::string gtestFilterParam_;
>  };
>  
>  Harness::Harness(const OptionsParser::Options &options)
> -	: options_(options)
> +	: options_(options), gtestArgv_(nullptr)
>  {
>  	cm_ = std::make_unique<CameraManager>();
>  }
> @@ -73,6 +85,8 @@ Harness::~Harness()
>  	Environment::instance()->destroy();
>  
>  	cm_->stop();
> +
> +	free(gtestArgv_);
>  }
>  
>  int Harness::init()
> @@ -86,6 +100,9 @@ int Harness::init()
>  		return ret;
>  	}
>  
> +	if (options_.isSet(OptList))
> +		return 0;
> +
>  	if (!options_.isSet(OptCamera)) {
>  		std::cout << "No camera specified, available cameras:" << std::endl;
>  		listCameras();
> @@ -118,12 +135,54 @@ void Harness::listCameras()
>  		std::cout << "- " << cam.get()->id() << std::endl;
>  }
>  
> +int Harness::buildGtestParameters(char *arg0)
> +{
> +	int argc = 0;
> +
> +	/*
> +	 * +2 to have space for both the 0th argument that is needed but not
> +	 * used and the null at the end.
> +	 */
> +	gtestArgv_ = (char**) malloc((gtestFlag_.size() + 2) * sizeof(char*));
> +	if (!gtestArgv_)
> +		return -ENOMEM;
> +
> +	gtestArgv_[argc] = arg0;
> +	argc++;
> +
> +	if (options_.isSet(OptList)) {
> +		gtestArgv_[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];
> +		gtestFilterParam_ = gtestFlag_.at("filter") + "=" + filter;
> +
> +		gtestArgv_[argc] = const_cast<char*>(gtestFilterParam_.c_str());
> +		argc++;
> +	}
> +
> +	gtestArgv_[argc] = 0;
> +	gtestArgc_ = argc;
> +
> +	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");
>  
> @@ -170,7 +229,12 @@ int main(int argc, char **argv)
>  	if (ret)
>  		return ret;
>  
> -	::testing::InitGoogleTest(&argc, argv);
> +	ret = harness.buildGtestParameters(argv[0]);
> +	if (ret)
> +		return ret;
> +	int gtestArgc = harness.gtestArgc();
> +
> +	::testing::InitGoogleTest(&gtestArgc, harness.gtestArgv());
>  	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
>  	return RUN_ALL_TESTS();
>  }
> -- 
> 2.31.1
> 

-- 
Regards,
Niklas Söderlund