test: Use test-listener rather than TestBase to assure quiescence after each method.

[jmarantz]

Description: Follow-up to #5811 -- use ::testing::TestEventListeners to add the singleton-quiescence hook. This is significantly cleaner than what I previously checked in which is to add a new TestBase class to every test-class, and covers TEST(fixture, name) tests too.

After this the TestBase class can be removed as it's now just an alias for ::testing:::Test.

Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

[alyssawilk]

Can you open a tracking issue and document why we need this and why it's a good idea? I think I could use a bit more context

Unit tests are supposed to be standalone - by default I'm not a big fan of anything changing global state on them

[htuch]

Please merge master to pick up #5827.

[alyssawilk]

Also have we looked into alternates? AFIK internal gtest has built in flagsavers - not sure if there's hooks for one off registration like you want or if they hard-coded in-house

Also on alternates, I'm a heavy user of gtest_filter/gunit_filter and losing TestSuite.Test is sad. would it be a pain to have our own macro function which called your function before test code rather than losing the standard gtest naming?

[jmarantz]

RE test-naming: let's call that a blocker for this PR; I was wondering how critical that was.

I have an alternate approach to interpose a new test-class. This transformation is more complicated (but tractable) to apply at scale. However, frequently in the codebase, we test class Foo with TEST(Foo, testname), and so we can't just add class Foo : public TestBase {}; and then add add the _F to the macro.

I'm also fine with defining a macro to replace TEST( but first we need to solve what transformation to make when the expected class-name is already a class. Maybe we can disambiguate without changing the testing UI by putting the test-class in a namespace? I'm open to suggestions.

[jmarantz]

@alyssawilk PTAL at the latest commits, in particular the new macro TEST_E (suggestions welcome on name) and the one file I reworked in that style, test/common/runtime/uuid_util_test.cc

This avoids changing any test or fixture names, but requires a broad syntactic change by putting the entire test-body in a macro arg.

Suggestions also welcome on how better to do this.

[mattklein123]

Suggestions also welcome on how better to do this.

Worth maybe talking to the googletest folks? I wonder if an upstream change might be the better way to do this? Some hook where we can use a custom base class for a TEST?

[alyssawilk]

+1 to what Matt said. We've managed to make changes to gmock for Envoy-specific asks, and I think this is a generically useful feature (which we either do use inside with a one-off or could). If we're not in a desperate hurry and gtest is amenable I think we could do this cleanly in O(weeks) and it seems better than updating every test name and/or custom closure semantics.

[alyssawilk]

Also on the other PR do we have checks in the fix_format script to not use the normal test-base? If not I strongly encourage adding them both for the last change and for whatever we do here.

[jmarantz]

RE "check_format" checks for avoiding "TEST(" -- yes, in this PR. For deriving test-classes from testing::Test? Yes, already in master.

RE "change gtest" -- that sounds reasonable. Just one other quick taste-test on something we could do with the current technology. WDYT of this transform:

old:

TEST(Foo, t1) { ... }
TEST(Foo, t2) { ... }

new:

class FooTest : public TestBase {};
TEST_F(FooTest, t1) { ... }
TEST_F(FooTest, t2) { ... }

This would work most of the time, and be achievable with automation. FooTest might collide here or there and would need to be resolved manually in the context of the PR. Would that be a decent outcome?

If not I'll contact the gtest team.

[alyssawilk]

How would you automate? Or you mean you could script the one time change?

FWIW I think you can also get away with aliasing FooTest to TestBase rather than declaring a new class and still have gtest filter work. I think I'd take that over the current macro though, so good call.

[jmarantz]

by aliasing do you mean using FooTest = TestBase;? I can try, but I'd be worried about two different tests with the same name, which are currently disambiguated by the test-suite.

Automation would be via one-time script.

[alyssawilk]

Yeah, I had concerns about that for the protocol integration test and turns out it works fine. I don't think creating a test class is bad though, so if you're more comfortable with that it's fine too.

I still think global test hooks would be better, but if it looks like it'll take more than a day or two I can buy the workaround described.

[alyssawilk]

Also since I'm a bit behind on reviews and annotations are nice :-)
/wait

[jmarantz]

Apologies for the size of the PR. You can see the pattern from the first visible file in it. This was all done semi-automatically, requiring manual-fixes where the test-fixture name matched an existing class name. My fix was to append 'Test' to the test fixture name, and I probably had to do this a few dozen times.

Also in this PR is a check_format addition to flag "TEST(" going forward. This check_format.py change is the last file in the PR so you can find the diff by going to the Files view and scrolling to the bottom.

[jmarantz]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5833 (comment) was created by @jmarantz.

see: more, trace.

[jmarantz]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5833 (comment) was created by @jmarantz.

see: more, trace.

[jmarantz]

@alyssawilk any further comments or discussion? This PR has the property that it probably won't be prone to automatically detected merge conflicts, but the longer it sits, the more new unchecked 'TEST(' macros will be added to the codebase. So if you let me know when you start reviewing I will do another quick merge and fix newly added 'TEST(' instances.

[alyssawilk]

Sorry, I started reviewing, paused to do some digging, and got distracted.

So digging
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md
Can we add an EnvoyTestListener which calls your custom code OnTestEnd and skip all of this work?

[jmarantz]

@alyssawilk yes done. Thank you very much for finding this...I had looked in some gtest header files for something like this and managed to miss it.

[alyssawilk]

Awesome - glad this worked out.

Both comments optional so I'll give you my LGTM now but I'm happy to do another pass if you fix either or both.

[alyssawilk]

Optional: think it's worth removing the check_format checks in this PR, just so folks can create new testing::Test classes while we get around to clean up?

[alyssawilk]

bonus points if you can think of a more descriptive name - I currently can't :-P

[jmarantz]

Thanks -- I think TestListener is the best I can do at the moment and I'll follow up this morning with the rollback of the existing check_format and other tests, so I'll go ahead and merge and get rid of that stuff :)