From 2377c8d32b82239a29bc81f452ab2875de810b2f Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Sun, 17 Oct 2021 12:53:30 +0200 Subject: [PATCH 1/9] Process filter string once instead of per test --- googletest/src/gtest.cc | 81 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 73 insertions(+), 8 deletions(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index d1ccd178..be97b2d2 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -748,6 +748,72 @@ bool UnitTestOptions::MatchesFilter(const std::string& name_str, return true; } +class Filter { + std::vector patterns_; + + public: + explicit Filter(const std::string& filter) { + if (filter.empty()) return; + + auto pattern_start = filter.begin(); + while (true) { + const auto delimiter = std::find(pattern_start, filter.end(), ':'); + patterns_.emplace_back(pattern_start, delimiter); + + if (delimiter == filter.end()) + break; + pattern_start = std::next(delimiter); + } + } + + bool MatchesName(const std::string& name) const { + const auto pattern_matches_name = [&name](const std::string& pattern) { + return PatternMatchesString(name, pattern.c_str(), + pattern.c_str() + pattern.size()); + }; + return std::any_of(patterns_.begin(), patterns_.end(), + pattern_matches_name); + } +}; + +class PositiveAndNegativeFilter { + Filter positive_filter_; + Filter negative_filter_; + + static std::pair + BreakIntoPositiveAndNegativeFilters(const std::string& filter) { + const auto dash_pos = filter.find('-'); + if (dash_pos == std::string::npos) { + return {filter, {}}; // Whole string is a positive filter + } else { + return {dash_pos ? filter.substr(0, dash_pos) : kUniversalFilter, + filter.substr(dash_pos + 1)}; + } + } + + PositiveAndNegativeFilter( + const std::pair& positive_and_negative_filters) + : positive_filter_(positive_and_negative_filters.first), + negative_filter_(positive_and_negative_filters.second) {} + + public: + explicit PositiveAndNegativeFilter(const std::string& filter) + : PositiveAndNegativeFilter(BreakIntoPositiveAndNegativeFilters(filter)) { + } + + bool MatchesTest(const std::string& test_suite_name, + const std::string& test_name) const { + const std::string& full_name = test_suite_name + "." + test_name.c_str(); + + return MatchesName(full_name); + } + + bool MatchesName(const std::string& name) const { + return positive_filter_.MatchesName(name) && + !negative_filter_.MatchesName(name); + } +}; + // Returns true if and only if the user-specified filter matches the test // suite name and the test name. bool UnitTestOptions::FilterMatchesTest(const std::string& test_suite_name, @@ -5754,9 +5820,9 @@ TestSuite* UnitTestImpl::GetTestSuite( auto* const new_test_suite = new TestSuite(test_suite_name, type_param, set_up_tc, tear_down_tc); + const Filter death_test_suite_filter{kDeathTestSuiteFilter}; // Is this a death test suite? - if (internal::UnitTestOptions::MatchesFilter(test_suite_name, - kDeathTestSuiteFilter)) { + if (death_test_suite_filter.MatchesName(test_suite_name)) { // Yes. Inserts the test suite after the last death test suite // defined so far. This only works when the test suites haven't // been shuffled. Otherwise we may end up running a death test @@ -6089,6 +6155,8 @@ int UnitTestImpl::FilterTests(ReactionToSharding shard_tests) { const int32_t shard_index = shard_tests == HONOR_SHARDING_PROTOCOL ? Int32FromEnvOrDie(kTestShardIndex, -1) : -1; + const PositiveAndNegativeFilter gtest_flag_filter{GTEST_FLAG_GET(filter)}; + const Filter disable_test_filter{kDisableTestFilter}; // num_runnable_tests are the number of tests that will // run across all shards (i.e., match filter and are not disabled). // num_selected_tests are the number of tests to be run on @@ -6104,14 +6172,11 @@ int UnitTestImpl::FilterTests(ReactionToSharding shard_tests) { const std::string test_name(test_info->name()); // A test is disabled if test suite name or test name matches // kDisableTestFilter. - const bool is_disabled = internal::UnitTestOptions::MatchesFilter( - test_suite_name, kDisableTestFilter) || - internal::UnitTestOptions::MatchesFilter( - test_name, kDisableTestFilter); + const bool is_disabled = disable_test_filter.MatchesName(test_suite_name) || + disable_test_filter.MatchesName(test_name); test_info->is_disabled_ = is_disabled; - const bool matches_filter = internal::UnitTestOptions::FilterMatchesTest( - test_suite_name, test_name); + const bool matches_filter = gtest_flag_filter.MatchesTest(test_suite_name, test_name); test_info->matches_filter_ = matches_filter; const bool is_runnable = From f5b4efef5f6a190c50437a81c2c4bfb1996eeb50 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Sun, 17 Oct 2021 18:44:54 +0200 Subject: [PATCH 2/9] Add comments describing the behavior of filters --- googletest/src/gtest.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index be97b2d2..c2e40f82 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -752,6 +752,7 @@ class Filter { std::vector patterns_; public: + // Constructs a filter form a string of patterns separated by `:`. explicit Filter(const std::string& filter) { if (filter.empty()) return; @@ -766,6 +767,8 @@ class Filter { } } + // Returns true if and only if name matches at least one of the patterns in + // the filter. bool MatchesName(const std::string& name) const { const auto pattern_matches_name = [&name](const std::string& pattern) { return PatternMatchesString(name, pattern.c_str(), @@ -790,17 +793,24 @@ class PositiveAndNegativeFilter { filter.substr(dash_pos + 1)}; } } - PositiveAndNegativeFilter( const std::pair& positive_and_negative_filters) : positive_filter_(positive_and_negative_filters.first), negative_filter_(positive_and_negative_filters.second) {} public: + // Constructs a positive and a negative filter from a string. The string + // contains a positive filter optionally followed by a '-' character and a + // negative filter. In case only a negative filter is provided the positive + // filter will be assumed "*". + // A filter is a list of patterns separated by ':'. explicit PositiveAndNegativeFilter(const std::string& filter) : PositiveAndNegativeFilter(BreakIntoPositiveAndNegativeFilters(filter)) { } + // Returns true if and only if test name (this is generated by appending test + // suit name and test name via a '.' character) matches the positive filter + // and does not match the negative filter. bool MatchesTest(const std::string& test_suite_name, const std::string& test_name) const { const std::string& full_name = test_suite_name + "." + test_name.c_str(); @@ -808,6 +818,8 @@ class PositiveAndNegativeFilter { return MatchesName(full_name); } + // Returns true if and only if name matches the positive filter and does not + // match the negative filter. bool MatchesName(const std::string& name) const { return positive_filter_.MatchesName(name) && !negative_filter_.MatchesName(name); From f20688737ad71746a013596e3798fa44b1abb6a2 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Sun, 17 Oct 2021 18:53:08 +0200 Subject: [PATCH 3/9] Get rid of redundant filter matching code --- googletest/src/gtest-internal-inl.h | 4 --- googletest/src/gtest.cc | 49 +---------------------------- 2 files changed, 1 insertion(+), 52 deletions(-) diff --git a/googletest/src/gtest-internal-inl.h b/googletest/src/gtest-internal-inl.h index 0b25d2f0..765d12d2 100644 --- a/googletest/src/gtest-internal-inl.h +++ b/googletest/src/gtest-internal-inl.h @@ -390,10 +390,6 @@ class GTEST_API_ UnitTestOptions { // This function is useful as an __except condition. static int GTestShouldProcessSEH(DWORD exception_code); #endif // GTEST_OS_WINDOWS - - // Returns true if "name" matches the ':' separated list of glob-style - // filters in "filter". - static bool MatchesFilter(const std::string& name, const char* filter); }; // Returns the current application's name, removing directory path if that diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index c2e40f82..e6140b74 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -723,31 +723,6 @@ static bool PatternMatchesString(const std::string& name_str, return true; } -bool UnitTestOptions::MatchesFilter(const std::string& name_str, - const char* filter) { - // The filter is a list of patterns separated by colons (:). - const char* pattern = filter; - while (true) { - // Find the bounds of this pattern. - const char* const next_sep = strchr(pattern, ':'); - const char* const pattern_end = - next_sep != nullptr ? next_sep : pattern + strlen(pattern); - - // Check if this pattern matches name_str. - if (PatternMatchesString(name_str, pattern, pattern_end)) { - return true; - } - - // Give up on this pattern. However, if we found a pattern separator (:), - // advance to the next pattern (skipping over the separator) and restart. - if (next_sep == nullptr) { - return false; - } - pattern = next_sep + 1; - } - return true; -} - class Filter { std::vector patterns_; @@ -830,31 +805,9 @@ class PositiveAndNegativeFilter { // suite name and the test name. bool UnitTestOptions::FilterMatchesTest(const std::string& test_suite_name, const std::string& test_name) { - const std::string& full_name = test_suite_name + "." + test_name.c_str(); - // Split --gtest_filter at '-', if there is one, to separate into // positive filter and negative filter portions - std::string str = GTEST_FLAG_GET(filter); - const char* const p = str.c_str(); - const char* const dash = strchr(p, '-'); - std::string positive; - std::string negative; - if (dash == nullptr) { - positive = str.c_str(); // Whole string is a positive filter - negative = ""; - } else { - positive = std::string(p, dash); // Everything up to the dash - negative = std::string(dash + 1); // Everything after the dash - if (positive.empty()) { - // Treat '-test1' as the same as '*-test1' - positive = kUniversalFilter; - } - } - - // A filter is a colon-separated list of patterns. It matches a - // test if any pattern in it matches the test. - return (MatchesFilter(full_name, positive.c_str()) && - !MatchesFilter(full_name, negative.c_str())); + return PositiveAndNegativeFilter{GTEST_FLAG_GET(filter)}.MatchesTest(test_suite_name, test_name); } #if GTEST_HAS_SEH From 3fc1ab66329ca90bfb5f1107a9cb22accabf0854 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Tue, 23 Nov 2021 15:16:45 +0100 Subject: [PATCH 4/9] Apply requested changes. --- googletest/src/gtest.cc | 72 +++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index e6140b74..259e7ce4 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -723,23 +723,16 @@ static bool PatternMatchesString(const std::string& name_str, return true; } -class Filter { - std::vector patterns_; - +namespace { +class UnitTestFilter { public: + UnitTestFilter() = default; + // Constructs a filter form a string of patterns separated by `:`. - explicit Filter(const std::string& filter) { + explicit UnitTestFilter(const std::string& filter) { if (filter.empty()) return; - auto pattern_start = filter.begin(); - while (true) { - const auto delimiter = std::find(pattern_start, filter.end(), ':'); - patterns_.emplace_back(pattern_start, delimiter); - - if (delimiter == filter.end()) - break; - pattern_start = std::next(delimiter); - } + SplitString(filter, ':', &patterns_); } // Returns true if and only if name matches at least one of the patterns in @@ -752,35 +745,33 @@ class Filter { return std::any_of(patterns_.begin(), patterns_.end(), pattern_matches_name); } + + private: + std::vector patterns_; }; -class PositiveAndNegativeFilter { - Filter positive_filter_; - Filter negative_filter_; - - static std::pair - BreakIntoPositiveAndNegativeFilters(const std::string& filter) { - const auto dash_pos = filter.find('-'); - if (dash_pos == std::string::npos) { - return {filter, {}}; // Whole string is a positive filter - } else { - return {dash_pos ? filter.substr(0, dash_pos) : kUniversalFilter, - filter.substr(dash_pos + 1)}; - } - } - PositiveAndNegativeFilter( - const std::pair& positive_and_negative_filters) - : positive_filter_(positive_and_negative_filters.first), - negative_filter_(positive_and_negative_filters.second) {} - +class PositiveAndNegativeUnitTestFilter { public: // Constructs a positive and a negative filter from a string. The string // contains a positive filter optionally followed by a '-' character and a // negative filter. In case only a negative filter is provided the positive // filter will be assumed "*". // A filter is a list of patterns separated by ':'. - explicit PositiveAndNegativeFilter(const std::string& filter) - : PositiveAndNegativeFilter(BreakIntoPositiveAndNegativeFilters(filter)) { + explicit PositiveAndNegativeUnitTestFilter(const std::string& filter) { + std::vector positive_and_negative_filters; + + SplitString(filter, '-', &positive_and_negative_filters); + const auto& positive_filter = positive_and_negative_filters.front(); + + if (positive_and_negative_filters.size() > 1) { + positive_filter_ = UnitTestFilter{ + positive_filter.size() ? positive_filter : kUniversalFilter}; + negative_filter_ = UnitTestFilter{positive_and_negative_filters.back()}; + } else { + // In case positive filter is empty + // we do not use kUniversalFilter by design + positive_filter_ = UnitTestFilter{positive_filter}; + } } // Returns true if and only if test name (this is generated by appending test @@ -799,7 +790,12 @@ class PositiveAndNegativeFilter { return positive_filter_.MatchesName(name) && !negative_filter_.MatchesName(name); } + + private: + UnitTestFilter positive_filter_; + UnitTestFilter negative_filter_; }; +} // namespace // Returns true if and only if the user-specified filter matches the test // suite name and the test name. @@ -807,7 +803,7 @@ bool UnitTestOptions::FilterMatchesTest(const std::string& test_suite_name, const std::string& test_name) { // Split --gtest_filter at '-', if there is one, to separate into // positive filter and negative filter portions - return PositiveAndNegativeFilter{GTEST_FLAG_GET(filter)}.MatchesTest(test_suite_name, test_name); + return PositiveAndNegativeUnitTestFilter{GTEST_FLAG_GET(filter)}.MatchesTest(test_suite_name, test_name); } #if GTEST_HAS_SEH @@ -5785,7 +5781,7 @@ TestSuite* UnitTestImpl::GetTestSuite( auto* const new_test_suite = new TestSuite(test_suite_name, type_param, set_up_tc, tear_down_tc); - const Filter death_test_suite_filter{kDeathTestSuiteFilter}; + const UnitTestFilter death_test_suite_filter{kDeathTestSuiteFilter}; // Is this a death test suite? if (death_test_suite_filter.MatchesName(test_suite_name)) { // Yes. Inserts the test suite after the last death test suite @@ -6120,8 +6116,8 @@ int UnitTestImpl::FilterTests(ReactionToSharding shard_tests) { const int32_t shard_index = shard_tests == HONOR_SHARDING_PROTOCOL ? Int32FromEnvOrDie(kTestShardIndex, -1) : -1; - const PositiveAndNegativeFilter gtest_flag_filter{GTEST_FLAG_GET(filter)}; - const Filter disable_test_filter{kDisableTestFilter}; + const PositiveAndNegativeUnitTestFilter gtest_flag_filter{GTEST_FLAG_GET(filter)}; + const UnitTestFilter disable_test_filter{kDisableTestFilter}; // num_runnable_tests are the number of tests that will // run across all shards (i.e., match filter and are not disabled). // num_selected_tests are the number of tests to be run on From d03d23a6e5ec58699955dd951957532e5aea3a84 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Fri, 3 Dec 2021 01:45:10 +0100 Subject: [PATCH 5/9] Reimplement MatchesFilter with new interfaces. --- googletest/src/gtest-internal-inl.h | 4 ++++ googletest/src/gtest.cc | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/googletest/src/gtest-internal-inl.h b/googletest/src/gtest-internal-inl.h index 765d12d2..0b25d2f0 100644 --- a/googletest/src/gtest-internal-inl.h +++ b/googletest/src/gtest-internal-inl.h @@ -390,6 +390,10 @@ class GTEST_API_ UnitTestOptions { // This function is useful as an __except condition. static int GTestShouldProcessSEH(DWORD exception_code); #endif // GTEST_OS_WINDOWS + + // Returns true if "name" matches the ':' separated list of glob-style + // filters in "filter". + static bool MatchesFilter(const std::string& name, const char* filter); }; // Returns the current application's name, removing directory path if that diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 259e7ce4..1537fd2c 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -797,6 +797,11 @@ class PositiveAndNegativeUnitTestFilter { }; } // namespace +bool UnitTestOptions::MatchesFilter(const std::string& name_str, + const char* filter) { + return UnitTestFilter{filter}.MatchesName(name_str); +} + // Returns true if and only if the user-specified filter matches the test // suite name and the test name. bool UnitTestOptions::FilterMatchesTest(const std::string& test_suite_name, From 4adbc9c9b24a9e9a9ac6ad18b3e43b4363b87eb8 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Fri, 17 Dec 2021 02:45:56 +0100 Subject: [PATCH 6/9] Apply requested changes to preserve old behavior. --- googletest/src/gtest.cc | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 1537fd2c..49fa896c 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -50,6 +50,7 @@ #include #include #include +#include #include // NOLINT #include #include @@ -724,26 +725,26 @@ static bool PatternMatchesString(const std::string& name_str, } namespace { + class UnitTestFilter { public: UnitTestFilter() = default; // Constructs a filter form a string of patterns separated by `:`. explicit UnitTestFilter(const std::string& filter) { - if (filter.empty()) return; - + // By design "" filter matches "" string. SplitString(filter, ':', &patterns_); } // Returns true if and only if name matches at least one of the patterns in // the filter. bool MatchesName(const std::string& name) const { - const auto pattern_matches_name = [&name](const std::string& pattern) { - return PatternMatchesString(name, pattern.c_str(), - pattern.c_str() + pattern.size()); - }; return std::any_of(patterns_.begin(), patterns_.end(), - pattern_matches_name); + [&name](const std::string& pattern) { + return PatternMatchesString( + name, pattern.c_str(), + pattern.c_str() + pattern.size()); + }); } private: @@ -766,10 +767,18 @@ class PositiveAndNegativeUnitTestFilter { if (positive_and_negative_filters.size() > 1) { positive_filter_ = UnitTestFilter{ positive_filter.size() ? positive_filter : kUniversalFilter}; - negative_filter_ = UnitTestFilter{positive_and_negative_filters.back()}; + + // TODO: Fail on multiple '-' characters + // For the moment to preserve old behavior we concatenate the rest of the + // string parts with `-` as separator to generate the negative filter. + negative_filter_ = UnitTestFilter{std::accumulate( + positive_and_negative_filters.begin() + 2, + positive_and_negative_filters.end(), positive_and_negative_filters[1], + [](const auto& lhs, const auto& rhs) { return lhs + '-' + rhs; })}; } else { - // In case positive filter is empty - // we do not use kUniversalFilter by design + // In case we don't have a negative filter and positive filter is "" + // we do not use kUniversalFilter by design as opposed to when we have a + // negative filter. positive_filter_ = UnitTestFilter{positive_filter}; } } @@ -779,9 +788,7 @@ class PositiveAndNegativeUnitTestFilter { // and does not match the negative filter. bool MatchesTest(const std::string& test_suite_name, const std::string& test_name) const { - const std::string& full_name = test_suite_name + "." + test_name.c_str(); - - return MatchesName(full_name); + return MatchesName(test_suite_name + "." + test_name); } // Returns true if and only if name matches the positive filter and does not From aea981dd54affc0220c94936084e5c0f1c206dd6 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Fri, 17 Dec 2021 02:49:45 +0100 Subject: [PATCH 7/9] Improve code readablity. --- googletest/src/gtest.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 49fa896c..959b3bbe 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -766,7 +766,7 @@ class PositiveAndNegativeUnitTestFilter { if (positive_and_negative_filters.size() > 1) { positive_filter_ = UnitTestFilter{ - positive_filter.size() ? positive_filter : kUniversalFilter}; + positive_filter.empty() ? kUniversalFilter : positive_filter}; // TODO: Fail on multiple '-' characters // For the moment to preserve old behavior we concatenate the rest of the @@ -774,7 +774,7 @@ class PositiveAndNegativeUnitTestFilter { negative_filter_ = UnitTestFilter{std::accumulate( positive_and_negative_filters.begin() + 2, positive_and_negative_filters.end(), positive_and_negative_filters[1], - [](const auto& lhs, const auto& rhs) { return lhs + '-' + rhs; })}; + [](const std::string& lhs, const std::string& rhs) { return lhs + '-' + rhs; })}; } else { // In case we don't have a negative filter and positive filter is "" // we do not use kUniversalFilter by design as opposed to when we have a From 4fc151ae6910c250329f72d9254b4a32634c696a Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Wed, 22 Dec 2021 16:22:56 +0100 Subject: [PATCH 8/9] Use normal for loop instead of accumulate. --- googletest/src/gtest.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 959b3bbe..54814d82 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -50,7 +50,6 @@ #include #include #include -#include #include // NOLINT #include #include @@ -771,10 +770,11 @@ class PositiveAndNegativeUnitTestFilter { // TODO: Fail on multiple '-' characters // For the moment to preserve old behavior we concatenate the rest of the // string parts with `-` as separator to generate the negative filter. - negative_filter_ = UnitTestFilter{std::accumulate( - positive_and_negative_filters.begin() + 2, - positive_and_negative_filters.end(), positive_and_negative_filters[1], - [](const std::string& lhs, const std::string& rhs) { return lhs + '-' + rhs; })}; + auto negative_filter_string = positive_and_negative_filters[1]; + for (std::size_t i = 2; i < positive_and_negative_filters.size(); i++) + negative_filter_string = + negative_filter_string + '-' + positive_and_negative_filters[i]; + negative_filter_ = UnitTestFilter{negative_filter_string}; } else { // In case we don't have a negative filter and positive filter is "" // we do not use kUniversalFilter by design as opposed to when we have a From 29bc520e5bf164e20fbb1b35d1fa9f148cf6a854 Mon Sep 17 00:00:00 2001 From: Hossein Ghahramanzadeh Date: Thu, 6 Jan 2022 15:46:11 +0100 Subject: [PATCH 9/9] Fix a typo in comments. --- googletest/src/gtest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc index 54814d82..ae611f9c 100644 --- a/googletest/src/gtest.cc +++ b/googletest/src/gtest.cc @@ -729,7 +729,7 @@ class UnitTestFilter { public: UnitTestFilter() = default; - // Constructs a filter form a string of patterns separated by `:`. + // Constructs a filter from a string of patterns separated by `:`. explicit UnitTestFilter(const std::string& filter) { // By design "" filter matches "" string. SplitString(filter, ':', &patterns_);