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

[Jacopo Mondi]

Hi Nicolas,

On Wed, Jun 16, 2021 at 10:25:35AM -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>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> 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 | 68 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp
> index 8c18845141de..bf9920008827 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::unique_ptr<CameraManager> cm_;
> +
> +	const std::map<std::string, std::string> gtestFlag_ = { { "list", "--gtest_list_tests" },
> +								{ "filter", "--gtest_filter" } };

Flags

>  };
>
>  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
> +	 */

Fits on one line

> +	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,58 @@ void Harness::listCameras()
>  		std::cout << "- " << cam.get()->id() << std::endl;
>  }
>
> +int Harness::initGtestParameters(char *arg0)
> +{
> +	int argc = 0;

Empty line

> +	/*
> +	 * +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 *[(gtestFlag_.size() + 2)];
> +	std::string filterParam;
> +
> +	if (!argv)
> +		return -ENOMEM;
> +
> +	argv[argc] = arg0;

argv[0]

Does ::testing::InitGoogleTest wants as argv[0] the main function
argv[0] ? I've no reasons to think it doesn't but I don't get why :)


> +	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
> +		 */
> +		filterParam = gtestFlag_.at("filter") + "=" +
> +			      static_cast<const std::string &>(options_[OptFilter]);
> +
> +		argv[argc] = const_cast<char *>(filterParam.c_str());
> +		argc++;
> +	}
> +
> +	argv[argc] = 0;
> +
> +	::testing::InitGoogleTest(&argc, argv);
> +
> +	delete argv;

I assume InitGoogleTest copies its arguments in then ?

The patch looks good, my only concern is that for any gtest option we
have to support we'll have to do the converion here. Do you know how
many options does gtest provides and how many of them could be useful
?

Thanks
  j

> +
> +	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 +207,5 @@ int main(int argc, char **argv)
>  	if (ret)
>  		return ret;
>
> -	return harness.run(argc, argv);
> +	return harness.run(argv[0]);
>  }
> --
> 2.32.0
>