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

[wrowe]

Replace [0-9] character group matches with \d supported by simple_re

This suggests we need Google Test to support Google re2
(and also should deprecate all std::regex in Envoy for re2)
but for the short term, solve these cases by simplifying the
pattern expressions. The one #ifdef presumes we wanted to
validate a non-'0' leading digit.

See: https://github.com/google/googletest/blob/587ceaeaee6c2ccb5e565858d7fe12aaf69795e6/googletest/include/gtest/gtest-death-test.h#L106

Signed-off-by: William A Rowe Jr [email protected]
Signed-off-by: Yechiel Kalmenson [email protected]
Signed-off-by: Sunjay Bhatia [email protected]

Risk Level: Low
Testing: Local on MSVC, Linux gcc
Docs Changes: n/a
Release Notes: n/a

[jmarantz]

Maybe this PR doesn't go far enough?

Alternatively, could you fix this infrastructurally by defining GTEST_HAS_POSIX_RE?

I found that by groveling around https://github.com/google/googletest/blob/master/googletest/include/gtest/internal/gtest-port.h

[wrowe]

Maybe this PR doesn't go far enough?

Alternatively, could you fix this infrastructurally by defining GTEST_HAS_POSIX_RE?

Sadly no, because we don't have posix regex. That doesn't change anything but cause additional failures building Google Test.

We are investigating, short term, adding PCRE in a portable and very predictable way. We are also following the Google Test rejected PR to adopt std::regex, which has already been rejected, but left open for further refinement to adopt Google RE2 which Envoy already consumes. That's the longer term fix, this is simply the band-aid

Apparently we need \d\+ and not \d+ on posix? This patch is stalled until we revert to windows specific flavors, or further debug the CI failures (I was under the impression we had this on Linux gcc but that may be incorrect )

[sunjayBhatia]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #9398 (comment) was created by @sunjayBhatia.

see: more, trace.

[wrowe]

@jmarantz if you could give this PR final approval, we simplified it in the short term. Longer term, the question is whether we want to adopt PCRE as yet another dependency, or if we want to wait on Google Test to adopt a Google RE2 alternative, which is already one of our dependencies.

[jmarantz]

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\\.");

[wrowe]

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

[wrowe]

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.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[sunjayBhatia]

Hey all, could we get a further review on this? Thanks!

[jmarantz]

OK, the sounds good.

@envoyproxy/senior-maintainers