[libcamera-devel,v3,4/4] lc-compliance: Add list and filter parameters
diff mbox series

Message ID 20210514131652.345486-5-nfraprado@collabora.com
State Superseded
Headers show
Series
  • lc-compliance: Refactor using Googletest
Related show

Commit Message

Nícolas F. R. A. Prado May 14, 2021, 1:16 p.m. UTC 
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@collabora.com>
---
 src/lc-compliance/main.cpp | 68 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

Comments

Niklas Söderlund May 16, 2021, 2:06 p.m. UTC | #1 
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@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
>
Nícolas F. R. A. Prado May 20, 2021, 5:46 p.m. UTC | #2 
Hi Niklas,

thanks for the feedback.

Em 2021-05-16 11:06, Niklas Söderlund escreveu:

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

You mean moving 

	::testing::InitGoogleTest(&gtestArgc, harness.gtestArgv());
	testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener);
	return RUN_ALL_TESTS();

inside the Harness function and removing those getters, right? Yeah, seems like
a good idea.

Thanks,
Nícolas

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

Patch
diff mbox series 

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_; };
 
 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();
 }