| Message ID | 20210607181506.51711-6-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thanks for your work. On 2021-06-07 15:15:06 -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> > --- > 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 | 69 +++++++++++++++++++++++++++++++++++--- > 1 file changed, 65 insertions(+), 4 deletions(-) > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > index 27503372d0eb..e35238471ee6 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::shared_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,59 @@ void Harness::listCameras() > std::cout << "- " << cam.get()->id() << std::endl; > } > > +int Harness::initGtestParameters(char *arg0) > +{ > + int argc = 0; > + char **argv; > + std::string filterParam; > + > + /* > + * +2 to have space for both the 0th argument that is needed but not > + * used and the null at the end. > + */ > + argv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *)); > + 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 > + */ > + const std::string &filter = options_[OptFilter]; I think 'filter' needs to be declared in the initGtestParameters() scope as it's consumed by ::testing::InitGoogleTest(). With this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Nice work! > + filterParam = gtestFlag_.at("filter") + "=" + filter; > + > + argv[argc] = const_cast<char *>(filterParam.c_str()); > + argc++; > + } > + > + argv[argc] = 0; > + > + ::testing::InitGoogleTest(&argc, argv); > + > + free(argv); > + > + 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 +208,5 @@ int main(int argc, char **argv) > if (ret) > return ret; > > - return harness.run(argc, argv); > + return harness.run(argv[0]); > } > -- > 2.31.1 >
Hi Niklas, On Wed, Jun 09, 2021 at 12:40:04AM +0200, Niklas Söderlund wrote: > Hi Nícolas, > > Thanks for your work. > > On 2021-06-07 15:15:06 -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> > > --- > > 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 | 69 +++++++++++++++++++++++++++++++++++--- > > 1 file changed, 65 insertions(+), 4 deletions(-) > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > index 27503372d0eb..e35238471ee6 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::shared_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,59 @@ void Harness::listCameras() > > std::cout << "- " << cam.get()->id() << std::endl; > > } > > > > +int Harness::initGtestParameters(char *arg0) > > +{ > > + int argc = 0; > > + char **argv; > > + std::string filterParam; > > + > > + /* > > + * +2 to have space for both the 0th argument that is needed but not > > + * used and the null at the end. > > + */ > > + argv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *)); > > + 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 > > + */ > > + const std::string &filter = options_[OptFilter]; > > I think 'filter' needs to be declared in the initGtestParameters() scope > as it's consumed by ::testing::InitGoogleTest(). With this fixed, Actually what is consumed in ::testing::InitGoogleTest() is filterParam, which indeed is in initGtestParameters()'s scope. This filter string here is used just to compose the entire string just below and move it to filterParam. I'm assuming when I do filterParam = gtestFlag_.at("filter") + "=" + filter; I'm copying the combined contents of all those strings to filterParam, so it should be safe to use in ::testing::InitGoogleTest(), but please tell me if this is not the case :). But maybe it would be better to just omit that local filter variable? A cast seems to be needed in that case: filterParam = gtestFlag_.at("filter") + "=" + (const std::string&) options_[OptFilter]; What do you think? Thanks, Nícolas > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Nice work! > > > + filterParam = gtestFlag_.at("filter") + "=" + filter; > > + > > + argv[argc] = const_cast<char *>(filterParam.c_str()); > > + argc++; > > + } > > + > > + argv[argc] = 0; > > + > > + ::testing::InitGoogleTest(&argc, argv); > > + > > + free(argv); > > + > > + 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 +208,5 @@ int main(int argc, char **argv) > > if (ret) > > return ret; > > > > - return harness.run(argc, argv); > > + return harness.run(argv[0]); > > } > > -- > > 2.31.1 > > > > -- > Regards, > Niklas Söderlund
Hi Nícolas, On 2021-06-09 09:30:40 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > On Wed, Jun 09, 2021 at 12:40:04AM +0200, Niklas Söderlund wrote: > > Hi Nícolas, > > > > Thanks for your work. > > > > On 2021-06-07 15:15:06 -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> > > > --- > > > 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 | 69 +++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 65 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > index 27503372d0eb..e35238471ee6 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::shared_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,59 @@ void Harness::listCameras() > > > std::cout << "- " << cam.get()->id() << std::endl; > > > } > > > > > > +int Harness::initGtestParameters(char *arg0) > > > +{ > > > + int argc = 0; > > > + char **argv; > > > + std::string filterParam; > > > + > > > + /* > > > + * +2 to have space for both the 0th argument that is needed but not > > > + * used and the null at the end. > > > + */ > > > + argv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *)); > > > + 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 > > > + */ > > > + const std::string &filter = options_[OptFilter]; > > > > I think 'filter' needs to be declared in the initGtestParameters() scope > > as it's consumed by ::testing::InitGoogleTest(). With this fixed, > > Actually what is consumed in ::testing::InitGoogleTest() is filterParam, which > indeed is in initGtestParameters()'s scope. This filter string here is used just > to compose the entire string just below and move it to filterParam. I'm assuming > when I do You are correct. > > filterParam = gtestFlag_.at("filter") + "=" + filter; > > I'm copying the combined contents of all those strings to filterParam, so it > should be safe to use in ::testing::InitGoogleTest(), but please tell me if this > is not the case :). > > But maybe it would be better to just omit that local filter variable? A cast > seems to be needed in that case: > > filterParam = gtestFlag_.at("filter") + "=" + > (const std::string&) options_[OptFilter]; > > What do you think? At least it would avoid me making a similar error again while reading the code :-) I think both solutions are ok. > > Thanks, > Nícolas > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > Nice work! > > > > > + filterParam = gtestFlag_.at("filter") + "=" + filter; > > > + > > > + argv[argc] = const_cast<char *>(filterParam.c_str()); > > > + argc++; > > > + } > > > + > > > + argv[argc] = 0; > > > + > > > + ::testing::InitGoogleTest(&argc, argv); > > > + > > > + free(argv); > > > + > > > + 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 +208,5 @@ int main(int argc, char **argv) > > > if (ret) > > > return ret; > > > > > > - return harness.run(argc, argv); > > > + return harness.run(argv[0]); > > > } > > > -- > > > 2.31.1 > > > > > > > -- > > Regards, > > Niklas Söderlund
diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp index 27503372d0eb..e35238471ee6 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::shared_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,59 @@ void Harness::listCameras() std::cout << "- " << cam.get()->id() << std::endl; } +int Harness::initGtestParameters(char *arg0) +{ + int argc = 0; + char **argv; + std::string filterParam; + + /* + * +2 to have space for both the 0th argument that is needed but not + * used and the null at the end. + */ + argv = (char **) malloc((gtestFlag_.size() + 2) * sizeof(char *)); + 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 + */ + const std::string &filter = options_[OptFilter]; + filterParam = gtestFlag_.at("filter") + "=" + filter; + + argv[argc] = const_cast<char *>(filterParam.c_str()); + argc++; + } + + argv[argc] = 0; + + ::testing::InitGoogleTest(&argc, argv); + + free(argv); + + 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 +208,5 @@ int main(int argc, char **argv) if (ret) return ret; - return harness.run(argc, argv); + return harness.run(argv[0]); }
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> --- 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 | 69 +++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 4 deletions(-)