From c99918ea5abe6a8cf7df43c97ba292a0df4e6861 Mon Sep 17 00:00:00 2001 From: Philip Top Date: Fri, 6 Oct 2023 08:38:47 -0700 Subject: [PATCH] add additional tests for coverage, (#928) Add some additional tests to try to get test coverage back to 100% refactor the positional parsing to reduce code duplication --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- cmake/CodeCoverage.cmake | 2 +- include/CLI/impl/App_inl.hpp | 94 +++++++++++++++------------------ include/CLI/impl/Config_inl.hpp | 26 ++++----- tests/AppTest.cpp | 17 +++++- tests/ConfigFileTest.cpp | 74 ++++++++++++++++++++++++++ tests/HelpTest.cpp | 16 ++++++ 6 files changed, 160 insertions(+), 69 deletions(-) diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake index e011ef13..1867b174 100644 --- a/cmake/CodeCoverage.cmake +++ b/cmake/CodeCoverage.cmake @@ -88,7 +88,7 @@ elseif(NOT CMAKE_COMPILER_IS_GNUCXX) endif() set(COVERAGE_COMPILER_FLAGS - "-g -O0 --coverage -fprofile-arcs -ftest-coverage -fno-inline -fno-inline-small-functions -fno-default-inline" + "-g -O0 --coverage -fprofile-arcs -ftest-coverage -fno-inline -fno-inline-small-functions -fno-default-inline -fno-elide-constructors" CACHE INTERNAL "") set(CMAKE_CXX_FLAGS_COVERAGE diff --git a/include/CLI/impl/App_inl.hpp b/include/CLI/impl/App_inl.hpp index 0fed48b0..2692e74b 100644 --- a/include/CLI/impl/App_inl.hpp +++ b/include/CLI/impl/App_inl.hpp @@ -82,7 +82,7 @@ CLI11_NODISCARD CLI11_INLINE char **App::ensure_utf8(char **argv) { CLI11_INLINE App *App::name(std::string app_name) { if(parent_ != nullptr) { - auto oname = name_; + std::string oname = name_; name_ = app_name; const auto &res = _compare_subcommand_names(*this, *_get_fallthrough_parent()); if(!res.empty()) { @@ -832,7 +832,7 @@ CLI11_NODISCARD CLI11_INLINE bool App::check_name(std::string name_to_check) con if(local_name == name_to_check) { return true; } - for(auto les : aliases_) { // NOLINT(performance-for-range-copy) + for(std::string les : aliases_) { // NOLINT(performance-for-range-copy) if(ignore_underscore_) { les = detail::remove_underscore(les); } @@ -1556,6 +1556,7 @@ CLI11_NODISCARD CLI11_INLINE bool App::_has_remaining_positionals() const { CLI11_INLINE bool App::_parse_positional(std::vector &args, bool haltOnSubcommand) { const std::string &positional = args.back(); + Option *posOpt{nullptr}; if(positionals_at_end_) { // deal with the case of required arguments at the end which should take precedence over other arguments @@ -1572,57 +1573,48 @@ CLI11_INLINE bool App::_parse_positional(std::vector &args, bool ha continue; } } - - parse_order_.push_back(opt.get()); - /// if we require a separator add it here - if(opt->get_inject_separator()) { - if(!opt->results().empty() && !opt->results().back().empty()) { - opt->add_result(std::string{}); - } - } - if(opt->get_trigger_on_parse() && - opt->current_option_state_ == Option::option_state::callback_run) { - opt->clear(); - } - opt->add_result(positional); - if(opt->get_trigger_on_parse()) { - opt->run_callback(); - } - args.pop_back(); - return true; + posOpt = opt.get(); + break; } } } } } - for(const Option_p &opt : options_) { - // Eat options, one by one, until done - if(opt->get_positional() && - (static_cast(opt->count()) < opt->get_items_expected_min() || opt->get_allow_extra_args())) { - if(validate_positionals_) { - std::string pos = positional; - pos = opt->_validate(pos, 0); - if(!pos.empty()) { - continue; + if(posOpt == nullptr) { + for(const Option_p &opt : options_) { + // Eat options, one by one, until done + if(opt->get_positional() && + (static_cast(opt->count()) < opt->get_items_expected_min() || opt->get_allow_extra_args())) { + if(validate_positionals_) { + std::string pos = positional; + pos = opt->_validate(pos, 0); + if(!pos.empty()) { + continue; + } } + posOpt = opt.get(); + break; } - if(opt->get_inject_separator()) { - if(!opt->results().empty() && !opt->results().back().empty()) { - opt->add_result(std::string{}); - } - } - if(opt->get_trigger_on_parse() && opt->current_option_state_ == Option::option_state::callback_run) { - opt->clear(); - } - opt->add_result(positional); - if(opt->get_trigger_on_parse()) { - opt->run_callback(); - } - parse_order_.push_back(opt.get()); - args.pop_back(); - return true; } } + if(posOpt != nullptr) { + parse_order_.push_back(posOpt); + if(posOpt->get_inject_separator()) { + if(!posOpt->results().empty() && !posOpt->results().back().empty()) { + posOpt->add_result(std::string{}); + } + } + if(posOpt->get_trigger_on_parse() && posOpt->current_option_state_ == Option::option_state::callback_run) { + posOpt->clear(); + } + posOpt->add_result(positional); + if(posOpt->get_trigger_on_parse()) { + posOpt->run_callback(); + } + + args.pop_back(); + return true; + } for(auto &subc : subcommands_) { if((subc->name_.empty()) && (!subc->disabled_)) { @@ -1964,7 +1956,7 @@ CLI11_INLINE void App::_trigger_pre_parse(std::size_t remaining_args) { } else if(immediate_callback_) { if(!name_.empty()) { auto pcnt = parsed_; - auto extras = std::move(missing_); + missing_t extras = std::move(missing_); clear(); parsed_ = pcnt; pre_parse_called_ = true; @@ -2150,12 +2142,12 @@ CLI11_INLINE void retire_option(App *app, Option *opt) { ->allow_extra_args(opt->get_allow_extra_args()); app->remove_option(opt); - auto *opt2 = app->add_option(option_copy->get_name(false, true), "option has been retired and has no effect") - ->type_name("RETIRED") - ->default_str("RETIRED") - ->type_size(option_copy->get_type_size_min(), option_copy->get_type_size_max()) - ->expected(option_copy->get_expected_min(), option_copy->get_expected_max()) - ->allow_extra_args(option_copy->get_allow_extra_args()); + auto *opt2 = app->add_option(option_copy->get_name(false, true), "option has been retired and has no effect"); + opt2->type_name("RETIRED") + ->default_str("RETIRED") + ->type_size(option_copy->get_type_size_min(), option_copy->get_type_size_max()) + ->expected(option_copy->get_expected_min(), option_copy->get_expected_max()) + ->allow_extra_args(option_copy->get_allow_extra_args()); Validator retired_warning{[opt2](std::string &) { std::cout << "WARNING " << opt2->get_name() << " is retired and has no effect\n"; diff --git a/include/CLI/impl/Config_inl.hpp b/include/CLI/impl/Config_inl.hpp index 8021d5f6..bf9f3908 100644 --- a/include/CLI/impl/Config_inl.hpp +++ b/include/CLI/impl/Config_inl.hpp @@ -221,15 +221,15 @@ inline std::vector ConfigBase::from_config(std::istream &input) cons } // Find = in string, split and recombine - auto pos = line.find(valueDelimiter); - if(pos != std::string::npos) { - name = detail::trim_copy(line.substr(0, pos)); - std::string item = detail::trim_copy(line.substr(pos + 1)); - auto cloc = item.find(commentChar); - if(cloc != std::string::npos) { - item.erase(cloc, std::string::npos); // NOLINT(readability-suspicious-call-argument) - detail::trim(item); - } + auto delimiter_pos = line.find_first_of(valueDelimiter); + auto comment_pos = line.find_first_of(commentChar); + if(comment_pos < delimiter_pos) { + delimiter_pos = std::string::npos; + } + if(delimiter_pos != std::string::npos) { + name = detail::trim_copy(line.substr(0, delimiter_pos)); + std::string item = detail::trim_copy(line.substr(delimiter_pos + 1, comment_pos - delimiter_pos - 1)); + if(item.size() > 1 && item.front() == aStart) { for(std::string multiline; item.back() != aEnd && std::getline(input, multiline);) { detail::trim(multiline); @@ -244,13 +244,7 @@ inline std::vector ConfigBase::from_config(std::istream &input) cons items_buffer = {item}; } } else { - name = detail::trim_copy(line); - auto cloc = name.find(commentChar); - if(cloc != std::string::npos) { - name.erase(cloc, std::string::npos); // NOLINT(readability-suspicious-call-argument) - detail::trim(name); - } - + name = detail::trim_copy(line.substr(0, comment_pos)); items_buffer = {"true"}; } if(name.find(parentSeparatorChar) == std::string::npos) { diff --git a/tests/AppTest.cpp b/tests/AppTest.cpp index 74a23c09..386418d4 100644 --- a/tests/AppTest.cpp +++ b/tests/AppTest.cpp @@ -27,7 +27,7 @@ TEST_CASE_METHOD(TApp, "OneFlagShortValues", "[app]") { run(); CHECK(app.count("-c") == 1u); CHECK(app.count("--count") == 1u); - auto v = app["-c"]->results(); + const auto &v = app["-c"]->results(); CHECK("v1" == v[0]); CHECK_THROWS_AS(app["--invalid"], CLI::OptionNotFound); @@ -1175,6 +1175,21 @@ TEST_CASE_METHOD(TApp, "PositionalAtEnd", "[app]") { CHECK_THROWS_AS(run(), CLI::ExtrasError); } +// Tests positionals at end +TEST_CASE_METHOD(TApp, "PositionalInjectSeparator", "[app]") { + std::string options; + std::vector> foo; + + app.add_option("-O", options); + auto *fooopt = app.add_option("foo", foo); + fooopt->inject_separator(); + args = {"test1", "-O", "Test", "test2"}; + run(); + + CHECK("Test" == options); + CHECK(foo.size() == 2U); +} + // Tests positionals at end TEST_CASE_METHOD(TApp, "RequiredPositionals", "[app]") { std::vector sources; diff --git a/tests/ConfigFileTest.cpp b/tests/ConfigFileTest.cpp index ba606dcf..5f689b5e 100644 --- a/tests/ConfigFileTest.cpp +++ b/tests/ConfigFileTest.cpp @@ -84,6 +84,7 @@ TEST_CASE("StringBased: FirstWithComments", "[config]") { ofile << "one=three\n"; ofile << "two=four\n"; ofile << "; and another one\n"; + ofile << " ; and yet another one\n"; ofile.seekg(0, std::ios::beg); @@ -1804,6 +1805,8 @@ TEST_CASE_METHOD(TApp, "IniNotConfigurable", "[config]") { } CHECK_THROWS_AS(run(), CLI::ConfigError); + app.allow_config_extras(CLI::config_extras_mode::ignore_all); + CHECK_NOTHROW(run()); } TEST_CASE_METHOD(TApp, "IniSubFailure", "[config]") { @@ -2072,6 +2075,66 @@ TEST_CASE_METHOD(TApp, "IniFlags", "[config]") { CHECK(five); } +TEST_CASE_METHOD(TApp, "IniFlagsComment", "[config]") { + TempFile tmpini{"TestIniTmp.ini"}; + app.set_config("--config", tmpini); + + { + std::ofstream out{tmpini}; + out << "[default]" << std::endl; + out << "two=2 # comment 1" << std::endl; + out << "three=true" << std::endl; + out << "four=on #comment 2" << std::endl; + out << "five #comment 3" << std::endl; + out << std::endl; + } + + int two{0}; + bool three{false}, four{false}, five{false}; + app.add_flag("--two", two); + app.add_flag("--three", three); + app.add_flag("--four", four); + app.add_flag("--five", five); + + run(); + + CHECK(two == 2); + CHECK(three); + CHECK(four); + CHECK(five); +} + +TEST_CASE_METHOD(TApp, "IniFlagsAltComment", "[config]") { + TempFile tmpini{"TestIniTmp.ini"}; + app.set_config("--config", tmpini); + + { + std::ofstream out{tmpini}; + out << "[default]" << std::endl; + out << "two=2 % comment 1" << std::endl; + out << "three=true" << std::endl; + out << "four=on %% comment 2" << std::endl; + out << "five %= 3" << std::endl; + out << std::endl; + } + + auto config = app.get_config_formatter_base(); + config->comment('%'); + int two{0}; + bool three{false}, four{false}, five{false}; + app.add_flag("--two", two); + app.add_flag("--three", three); + app.add_flag("--four", four); + app.add_flag("--five", five); + + run(); + + CHECK(two == 2); + CHECK(three); + CHECK(four); + CHECK(five); +} + TEST_CASE_METHOD(TApp, "IniFalseFlags", "[config]") { TempFile tmpini{"TestIniTmp.ini"}; app.set_config("--config", tmpini); @@ -2299,6 +2362,17 @@ TEST_CASE_METHOD(TApp, "TomlOutputShortSingleDescription", "[config]") { CHECK_THAT(str, Contains("# " + description + "\n" + flag + "=false\n")); } +TEST_CASE_METHOD(TApp, "TomlOutputdefaultOptionString", "[config]") { + std::string option = "some_option"; + const std::string description = "Some short description."; + app.add_option("--" + option, description)->run_callback_for_default(); + + run(); + + std::string str = app.config_to_str(true, true); + CHECK_THAT(str, Contains("# " + description + "\n" + option + "=\"\"\n")); +} + TEST_CASE_METHOD(TApp, "TomlOutputShortDoubleDescription", "[config]") { std::string flag1 = "flagnr1"; std::string flag2 = "flagnr2"; diff --git a/tests/HelpTest.cpp b/tests/HelpTest.cpp index 4af0a3b8..a061b989 100644 --- a/tests/HelpTest.cpp +++ b/tests/HelpTest.cpp @@ -1336,3 +1336,19 @@ TEST_CASE("TVersion: parse_throw", "[help]") { CHECK(1U == cptr->count()); } } + +TEST_CASE("TVersion: exit", "[help]") { + + CLI::App app; + + app.set_version_flag("--version", CLI11_VERSION); + + try { + app.parse("--version"); + } catch(const CLI::CallForVersion &v) { + std::ostringstream out; + auto ret = app.exit(v, out); + CHECK_THAT(out.str(), Contains(CLI11_VERSION)); + CHECK(0 == ret); + } +}