Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Google Test simple_re doesn't support [0-9] patterns #9398

Merged
merged 3 commits into from
Dec 31, 2019
Merged

Google Test simple_re doesn't support [0-9] patterns #9398

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23d9275
Google Test simple_re doesn't support [0-9] patterns
wrowe Dec 16, 2019
9d26bf2
Restore regex numeric pattern matching
wrowe Dec 19, 2019
9c97864
Full coverage for nonzero number when using simple re
sunjayBhatia Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/common/common/regex_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ TEST(Utility, ParseRegex) {
matcher.mutable_google_re2()->mutable_max_program_size()->set_value(1);
matcher.set_regex("/asdf/.*");
EXPECT_THROW_WITH_REGEX(Utility::parseRegex(matcher), EnvoyException,
"RE2 program size of [0-9]+ > max program size of 1\\.");
"RE2 program size of \\d+ > max program size of 1\\.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT of factoring out:

#ifdef GTEST_USES_SIMPLE_RE
#define GTEST_RE_DIGITS "\\d+"
#else 
#define GTEST_RE_DIGITS "[0-9]+"
#endif

And then you can simplify the use like:

    EXPECT_THROW_WITH_REGEX(Utility::parseRegex(matcher), EnvoyException,
                            "RE2 program size of " GTEST_RE_DIGITS " > max program size of 1\\.");	                            "RE2 program size of [0-9]+ > max program size of 1\\.");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the concept, but not clear that it solves the one issue, e.g. "[1-9][0-9]*" vs "\d+" for a non-zero value. I guess we can have a second macro.

I'd prefer we code this as currently proposed. Once a portable, non-SIMPLE_RE solution is present, we will simplify the logic, but we need Google Test upstream to solve this first. Depending on that solution, we may be choosing \d in place of [0-9] for legibility, if the RE solution is portable to both (e.g. using either PCRE or RE2.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an argument against...
"datetime=" GTEST_RE_DIGITS "-" GTEST_RE_DIGITS "-" GTEST_RE_DIGITS "T" GTEST_RE_DIGITS ":" GTEST_RE_DIGITS ":" GTEST_RE_DIGITS "\." GTEST_RE_DIGITS "+Z nonzeronum=" GTEST_RE_DIGITS_NONZERO ""));
is rather dreadful to read.

}
}

Expand Down
6 changes: 6 additions & 0 deletions test/common/tcp_proxy/tcp_proxy_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,9 +1564,15 @@ TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(AccessLogBytesRxTxDuration)) {
upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose);
filter_.reset();

#if !defined(WIN32)
EXPECT_THAT(access_log_data_,
MatchesRegex(
"bytesreceived=1 bytessent=2 datetime=[0-9-]+T[0-9:.]+Z nonzeronum=[1-9][0-9]*"));
wrowe marked this conversation as resolved.
Show resolved Hide resolved
#else
EXPECT_THAT(access_log_data_,
MatchesRegex("bytesreceived=1 bytessent=2 "
"datetime=\\d+-\\d+-\\d+T\\d+:\\d+:\\d+\\.\\d+Z nonzeronum=\\d+"));
#endif
}

TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(AccessLogUpstreamSSLConnection)) {
Expand Down
6 changes: 3 additions & 3 deletions test/integration/tcp_proxy_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,16 +278,16 @@ TEST_P(TcpProxyIntegrationTest, AccessLog) {

// Regex matching localhost:port
const std::string ip_port_regex = (GetParam() == Network::Address::IpVersion::v4)
? R"EOF(127\.0\.0\.1:[0-9]+)EOF"
: R"EOF(\[::1\]:[0-9]+)EOF";
? R"EOF(127\.0\.0\.1:\d+)EOF"
: R"EOF(\[::1\]:\d+)EOF";

const std::string ip_regex =
(GetParam() == Network::Address::IpVersion::v4) ? R"EOF(127\.0\.0\.1)EOF" : R"EOF(::1)EOF";

// Test that all three addresses were populated correctly. Only check the first line of
// log output for simplicity.
EXPECT_THAT(log_result,
MatchesRegex(fmt::format("upstreamlocal={0} upstreamhost={0} downstream={1}\n.*",
MatchesRegex(fmt::format("upstreamlocal={0} upstreamhost={0} downstream={1}\r?\n.*",
ip_port_regex, ip_regex)));
}

Expand Down