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

12615 diff mbox series

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>
Reviewed-by: Niklas Söderlund <niklas.soderlund@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(-)

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@collabora.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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
>

Hi Nicolas,

Thank you for the patch.

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@collabora.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@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" } };
>  };
>  
>  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
> +	 */
> +	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;
> +	/*
> +	 * +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;
> +	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'm getting error: 'delete' applied to a pointer that was allocated with
'new[]'; did you mean 'delete[]'? [-Werror,-Wmismatched-new-delete] here
on clang.


Paul

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

Hi Jacopo,

On Fri, Jun 18, 2021 at 12:09:58AM +0200, Jacopo Mondi wrote:
> 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@collabora.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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]

This and all of the above fixed.

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

Well, it's not that gtest wants argv[0] to do something useful, but it does
expect something there. I've tried skipping setting argv[0], but then gtest
crashes, since its parameter parsing routine probably expects a valid char
pointer there. But I don't think the string really matters.

So it was either passing argv[0] from main to it or creating an empty string to
pass it, and I thought the former would be less confusing even though it's more
work for something not useful (at least not to my knowledge). I could certainly
do the latter instead if you think that's better.

And at the time I didn't realize InitGoogleTest() copied the parameters
internally, so I thought we'd need to keep the empty string on the Harness class
scope. But since it does we could just create an empty string local to this
function and use that as arg0, which is way better.

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

Yep.

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

These are Gtest's options (from its 'help'):

    Test Selection:
      --gtest_list_tests
          List the names of all tests instead of running them. The name of
          TEST(Foo, Bar) is "Foo.Bar".
      --gtest_filter=POSITIVE_PATTERNS[-NEGATIVE_PATTERNS]
          Run only the tests whose name matches one of the positive patterns but
          none of the negative patterns. '?' matches any single character; '*'
          matches any substring; ':' separates two patterns.
      --gtest_also_run_disabled_tests
          Run all disabled tests too.
    
    Test Execution:
      --gtest_repeat=[COUNT]
          Run the tests repeatedly; use a negative count to repeat forever.
      --gtest_shuffle
          Randomize tests' orders on every iteration.
      --gtest_random_seed=[NUMBER]
          Random number seed to use for shuffling test orders (between 1 and
          99999, or 0 to use a seed based on the current time).
    
    Test Output:
      --gtest_color=(yes|no|auto)
          Enable/disable colored output. The default is auto.
      --gtest_brief=1
          Only print test failures.
      --gtest_print_time=0
          Don't print the elapsed time of each test.
      --gtest_output=(json|xml)[:DIRECTORY_PATH/|:FILE_PATH]
          Generate a JSON or XML report in the given directory or with the given
          file name. FILE_PATH defaults to test_detail.xml.
      --gtest_stream_result_to=HOST:PORT
          Stream test results to the given server.
    
    Assertion Behavior:
      --gtest_death_test_style=(fast|threadsafe)
          Set the default death test style.
      --gtest_break_on_failure
          Turn assertion failures into debugger break-points.
      --gtest_throw_on_failure
          Turn assertion failures into C++ exceptions for use by an external
          test framework.
      --gtest_catch_exceptions=0
          Do not report exceptions as test failures. Instead, allow them
          to crash the program or throw a pop-up (on Windows).
    
    Except for --gtest_list_tests, you can alternatively set the corresponding
    environment variable of a flag (all letters in upper-case). For example, to
    disable colored text output, you can either specify --gtest_color=no or set
    the GTEST_COLOR environment variable to no.

There are quite a few, but most don't seem that useful to me personally.

Thanks,
Nícolas

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

Hi Paul,

On Mon, Jun 21, 2021 at 07:56:25PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> 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@collabora.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@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" } };
> >  };
> >  
> >  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
> > +	 */
> > +	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;
> > +	/*
> > +	 * +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;
> > +	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'm getting error: 'delete' applied to a pointer that was allocated with
> 'new[]'; did you mean 'delete[]'? [-Werror,-Wmismatched-new-delete] here
> on clang.

Thanks for the feedback. I was having some trouble with clang so I didn't test
with it. But I'm now able to compile with it for some reason and I fixed that
error, so we should be good with clang for v9. (CC: Laurent, it does say
"Library libc++ found: NO" here).

Thanks,
Nícolas

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

Hi Nícolas,

On Mon, Jun 21, 2021 at 05:01:37PM -0300, Nícolas F. R. A. Prado wrote:
> On Mon, Jun 21, 2021 at 07:56:25PM +0900, paul.elder@ideasonboard.com wrote:
> > 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@collabora.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@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" } };
> > >  };
> > >  
> > >  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
> > > +	 */
> > > +	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;
> > > +	/*
> > > +	 * +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;
> > > +	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'm getting error: 'delete' applied to a pointer that was allocated with
> > 'new[]'; did you mean 'delete[]'? [-Werror,-Wmismatched-new-delete] here
> > on clang.
> 
> Thanks for the feedback. I was having some trouble with clang so I didn't test
> with it. But I'm now able to compile with it for some reason and I fixed that
> error, so we should be good with clang for v9. (CC: Laurent, it does say
> "Library libc++ found: NO" here).

That's not a blocker, it falls back to libstdc++ then (the
implementation from gcc). We don't test that combination much though, so
it's nice to know it's working for you :-)

> > > +
> > > +	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]);
> > >  }