cache filter: mock cache in CacheFilterTest by mkbehr · Pull Request #20593 · envoyproxy/envoy

mkbehr · 2022-03-30T17:13:18Z

Commit Message: Refactor CacheFilterTest to use a mock cache.
Additional Description: Add a mock cache and associated objects, and restructure the CacheFilter unit test to use them instead of SimpleHttpCache. This focuses the test on specifically exercising the CacheFilter's interface, and lets tests skip the extra requests they were sending to set up the cache for a lookup or verify the results of an insert. It'll also let future tests exercise more fine-grained control over the cache's behavior, e.g. to test flow control.
Risk Level: Low
Testing: bazel test //test/extensions/filters/http/cache/...
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Michael Behr <mkbehr@google.com>

repokitteh-read-only · 2022-03-30T17:13:21Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20593 was opened by mkbehr.

see: more, trace.

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr · 2022-03-30T20:02:25Z

Presubmit failures look to be an upstream Bazel bug; I filed issue #20599 for them.

adisuissa · 2022-03-31T14:03:10Z

cc @toddmgreer @jmarantz @penguingao @mpwarres @capoferro as codeowners

toddmgreer · 2022-04-01T04:38:11Z

test/extensions/filters/http/cache/cache_filter_test.cc

+  }

-  SimpleHttpCache simple_cache_;
+  std::shared_ptr<StrictMock<MockHttpCache>> cache_;


Please document why this test needs the additional complexity of using a mock and having to respecify its behavior.

Added a test fixture comment. This is most important for flow control tests in a future change - I can imagine splitting the test fixture to let the tests that don't need as fine-grained control use a fake, but for now this is at least an improvement over specifying the fake's behavior via extra copies of CacheFilter.

It would make sense to me to add such a test in this PR to demonstrate the value, if that seems feasible.

I'm always skeptical with complex mocks whether they are mocking the right behavior, and wonder why (without looking too deeply) you think that's an improvement over using the CacheFilter itself.

It's hard to add those tests before the CacheFilter implements its flow control logic: right now we can't test e.g. downstream backpressure while serving from cache because the whole response gets buffered, so there aren't any signals from the connection manager that we can interleave with cache body fragments.

I take your point about it being hard to trust that complex mocks are doing the right thing. My thinking is that when it comes to setting up the environment and reading results, it's even worse to trust that the code under test is doing the right thing. So in the existing state of the test, we test CacheFilter inserts by using CacheFilter lookups to verify the state of the cache, and we test lookups by using inserts to set up the cache, but we can never verify the behavior of one of those operations independently of the other. So there's a class of bugs that affect both insert and lookup that the existing test setup can never catch.

Testing inserts and lookups independently is not a goal, because they have practically no independently defined behavior. The (simplified) spec is that if you insert something then look it up, you get the response you inserted (with certain modifications). We don't use CacheFilter lookups to verify the state of the cache--we use CacheFilter lookups to verify that the observable behavior of a particular sequence of operations is correct. If some "bug" doesn't change the observable behavior, then it isn't really a bug.

There are times when it is helpful to write tests that are less faithful to deployed reality, and testing a slow HttpCache is probably such a time. However, this PR changes all of the tests in this file to using mocks, including those that are better off using the real thing. Perhaps CacheFilterTest should have RealCacheFilterTest and MockCacheFilterTest subclasses, or should use a simple mock that defaults to letting SimpleHttpCache handle everything, with ON_CALLs only appearing in specific tests?

In the past I've done some amount of insert/lookup testing against stats held in the cache object. That allows some modest testing of insert/lookup in isolation. E.g. you could verify that inserts fill the cache with data without doing a lookup. You could verify that lookups fail due to expiry. This doesn't require mocks.

@mbehr maybe a spec of what your testing plan is would make it easier to understand the future value of the mocks.

toddmgreer · 2022-04-01T04:40:10Z

source/extensions/filters/http/cache/http_cache.h


  // Headers of the cached response.
-  Http::ResponseHeaderMapPtr headers_;
+  Http::ResponseHeaderMapPtr headers_ = nullptr;


I think the preferred way in Envoy is to rely on default construction. (Please correct me if I'm mistaken.)

That way doesn't compile for me; breaks on -Wmissing-field-initializers.

did this get resolved somehow? Why is it compiling without initializers without your PR? What changed?

Nothing ever constructed a LookupResult before this, just declared it with everything uninitialized and then set its fields. Is there a problem with doing it this way? We could explicitly define a constructor instead.

I don't understand why it doesn't work to just revert all the initializers in this class and let them default-construct. You mentioned that doesn't compile for you which seems strange because it works in main today.

What doesn't compile is the new constructor calls to LookupResult that I added in CacheFilterTest. They can't default-construct without these changes because -Wmissing-field-initializers prevents that. They weren't default-constructing before because the existing usage initialized them to indeterminate values.

Yeah I bet if you add std::move you have to have a lot more detailed constructors. What's the implication if you drop that?

Other than that, after a quick scan, I just noticed a bunch of new code where you pass LookupResult by value and it might work better to pass by const ref.

Signed-off-by: Michael Behr <mkbehr@google.com>

…-filter-test-mock-cache Signed-off-by: Michael Behr <mkbehr@google.com>

wrowe · 2022-04-05T23:50:37Z

Definitely breakage in this patch, not main;

wrowe · 2022-04-05T23:51:15Z

test/extensions/filters/http/cache/cache_filter_test.cc: In member function ‘std::unique_ptr<Envoy::Extensions::HttpFilters::Cache::MockInsertContext, std::default_delete<Envoy::Extensions::HttpFilters::Cache::MockInsertContext> > Envoy::Extensions::HttpFilters::Cache::{anonymous}::CacheFilterTest::headerOnlyInsertContext() const’:
test/extensions/filters/http/cache/cache_filter_test.cc:188:21: error: redundant move in return statement [-Werror=redundant-move]
  188 |     return std::move(insert_context);
      |            ~~~~~~~~~^~~~~~~~~~~~~~~~
test/extensions/filters/http/cache/cache_filter_test.cc:188:21: note: remove ‘std::move’ call
test/extensions/filters/http/cache/cache_filter_test.cc: In member function ‘std::unique_ptr<Envoy::Extensions::HttpFilters::Cache::MockInsertContext, std::default_delete<Envoy::Extensions::HttpFilters::Cache::MockInsertContext> > Envoy::Extensions::HttpFilters::Cache::{anonymous}::CacheFilterTest::bodyInsertContext(const std::vector<absl::string_view>&) const’:
test/extensions/filters/http/cache/cache_filter_test.cc:214:21: error: redundant move in return statement [-Werror=redundant-move]
  214 |     return std::move(insert_context);
      |            ~~~~~~~~~^~~~~~~~~~~~~~~~
test/extensions/filters/http/cache/cache_filter_test.cc:214:21: note: remove ‘std::move’ call
test/extensions/filters/http/cache/cache_filter_test.cc: In member function ‘std::unique_ptr<Envoy::Extensions::HttpFilters::Cache::MockInsertContext, std::default_delete<Envoy::Extensions::HttpFilters::Cache::MockInsertContext> > Envoy::Extensions::HttpFilters::Cache::{anonymous}::CacheFilterTest::headerOnlyInsertContext() const’:
test/extensions/filters/http/cache/cache_filter_test.cc:188:21: error: redundant move in return statement [-Werror=redundant-move]
  188 |     return std::move(insert_context);
      |            ~~~~~~~~~^~~~~~~~~~~~~~~~
test/extensions/filters/http/cache/cache_filter_test.cc:188:21: note: remove ‘std::move’ call
test/extensions/filters/http/cache/cache_filter_test.cc: In member function ‘std::unique_ptr<Envoy::Extensions::HttpFilters::Cache::MockInsertContext, std::default_delete<Envoy::Extensions::HttpFilters::Cache::MockInsertContext> > Envoy::Extensions::HttpFilters::Cache::{anonymous}::CacheFilterTest::bodyInsertContext(const std::vector<absl::string_view>&) const’:
test/extensions/filters/http/cache/cache_filter_test.cc:214:21: error: redundant move in return statement [-Werror=redundant-move]
  214 |     return std::move(insert_context);
      |            ~~~~~~~~~^~~~~~~~~~~~~~~~
test/extensions/filters/http/cache/cache_filter_test.cc:214:21: note: remove ‘std::move’ call

wrowe · 2022-04-05T23:51:22Z

/wait

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr · 2022-04-06T20:00:56Z

Thanks for catching that - looks like #20599 was masking that breakage. Should be fixed now.

jmarantz · 2022-04-12T18:35:29Z

Can we have @toddmgreer or @capoferro take a look first and then I'l do a final pass?

capoferro

LGTM. Thanks!

test/extensions/filters/http/cache/cache_filter_test.cc

jmarantz

lgtm mostly - the largest problem is I haven't been sold on the end-goal of these mocks being better than the current state, but I'm OK with it.

jmarantz · 2022-04-18T13:48:09Z

source/extensions/filters/http/cache/http_cache.h


  // Headers of the cached response.
-  Http::ResponseHeaderMapPtr headers_;
+  Http::ResponseHeaderMapPtr headers_ = nullptr;


did this get resolved somehow? Why is it compiling without initializers without your PR? What changed?

jmarantz · 2022-04-18T13:49:06Z

test/extensions/filters/http/cache/cache_filter_test.cc

-    filter->setDecoderFilterCallbacks(decoder_callbacks_);
-    filter->setEncoderFilterCallbacks(encoder_callbacks_);
-    return filter;
+constexpr absl::string_view kBody = "abc";


envoy style convention for constants would use Body here but I'd suggest a slightly longer name for a file-scoped constant. Maybe CacheResponseBody?

jmarantz · 2022-04-18T13:54:01Z

test/extensions/filters/http/cache/cache_filter_test.cc

  void SetUp() override {
-    ON_CALL(decoder_callbacks_, dispatcher()).WillByDefault(::testing::ReturnRef(*dispatcher_));
+    cache_ = std::make_shared<StrictMock<MockHttpCache>>();
+    filter_ = makeFilter();


this was pre-existing, but why not just put all this in the CacheFilterTest ctor? Ditto for TearDown vs dtor.

jmarantz · 2022-04-18T13:56:20Z

test/extensions/filters/http/cache/cache_filter_test.cc

+  void expectCacheHitNoBody() {
+    // The filter should ask the cache for a lookup context, which will return a
+    // headers-only hit.
+    expectLookup({/*cache_entry_status_=*/CacheEntryStatus::Ok,


The /*cache_entry_status_=*/ annotation seems unnecessary given the well-qualified enum, here & below.

jmarantz · 2022-04-18T13:58:00Z

test/extensions/filters/http/cache/cache_filter_test.cc

+          std::make_unique<Envoy::Http::TestResponseHeaderMapImpl>(response_headers);
+      response_headers_from_cache->setCopy(Http::CustomHeaders::get().Age, age);
+      cb({/*cache_entry_status_=*/cache_entry_status,
+          /*headers_=*/std::move(response_headers_from_cache),


all these comments seem unnecessary in this case and just clutter the code.

We don't generally have these in Envoy style though there were cases above where you were explaining what the nullptr was for, and that case makes sense to me.

sg, removed all of these that could be inferred from context.

jmarantz · 2022-04-18T14:01:09Z

test/extensions/filters/http/cache/cache_filter_test.cc

+  }

-  SimpleHttpCache simple_cache_;
+  std::shared_ptr<StrictMock<MockHttpCache>> cache_;


It would make sense to me to add such a test in this PR to demonstrate the value, if that seems feasible.

I'm always skeptical with complex mocks whether they are mocking the right behavior, and wonder why (without looking too deeply) you think that's an improvement over using the CacheFilter itself.

jmarantz · 2022-04-18T14:08:00Z

test/extensions/filters/http/cache/cache_filter_test.cc

-    filter->onDestroy();
-  }
+  // Add Etag & Last-Modified headers to the response for validation
+  response_headers_.setReferenceKey(Envoy::Http::CustomHeaders::get().Etag, etag);


optional: this all might be easier to read (and slightly faster) if you put Envoy::Http::CustomHeaders::get() into a temp reference.

jmarantz · 2022-04-18T14:10:25Z

/wait

Signed-off-by: Michael Behr <mkbehr@google.com>

jmarantz · 2022-04-18T20:46:45Z

test/extensions/filters/http/cache/cache_filter_test.cc

-        encodeHeaders_(testing::AllOf(IsSupersetOfHeaders(response_headers_),
-                                      HeaderHasValueRef(Http::CustomHeaders::get().Age, age)),
-                       true));
+  void expectLookup(LookupResult lookup_result,


jmarantz · 2022-04-18T20:47:27Z

test/extensions/filters/http/cache/cache_filter_test.cc

+                    std::function<void(MockLookupContext&)> context_callback = nullptr) {
+    // Wrap the move-only LookupResult in a copyable shared_ptr to fit with
+    // EXPECT_CALL(...).WillOnce()'s interface.
+    auto result_wrapper = std::make_shared<LookupResult>(std::move(lookup_result));


will everything be cleaner if you just drop the std::move here?

jmarantz · 2022-04-18T20:47:36Z

test/extensions/filters/http/cache/cache_filter_test.cc

+    return lookup_context;
+  }
+
+  std::unique_ptr<MockLookupContext> makeLookupContext(LookupResult result) {


jmarantz · 2022-04-18T20:47:42Z

test/extensions/filters/http/cache/cache_filter_test.cc

+  // Returns a std::function that can be used as an action with
+  // MockLookupContext::getHeaders to find response_headers_, with other fields
+  // of the LookupResult as specified.
+  std::function<void(LookupHeadersCallback)> getHeaders(LookupResult lookup_result) {


jmarantz · 2022-04-18T20:49:10Z

source/extensions/filters/http/cache/http_cache.h


  // Headers of the cached response.
-  Http::ResponseHeaderMapPtr headers_;
+  Http::ResponseHeaderMapPtr headers_ = nullptr;


Yeah I bet if you add std::move you have to have a lot more detailed constructors. What's the implication if you drop that?

Other than that, after a quick scan, I just noticed a bunch of new code where you pass LookupResult by value and it might work better to pass by const ref.

jmarantz · 2022-04-19T13:36:24Z

/wait

github-actions · 2022-05-19T16:08:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 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!

github-actions · 2022-05-26T20:01:21Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Refactor CacheFilterTest to use a mock cache.

1584757

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr added 2 commits March 30, 2022 13:14

remove whitespace

f1335a1

Signed-off-by: Michael Behr <mkbehr@google.com>

spelling

56cffe4

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr mentioned this pull request Mar 30, 2022

CI test builds failing with "invalid action cache entry" #20599

Closed

mkbehr marked this pull request as ready for review March 30, 2022 20:01

mkbehr requested a review from jmarantz as a code owner March 30, 2022 20:01

toddmgreer suggested changes Apr 1, 2022

View reviewed changes

mkbehr added 2 commits April 5, 2022 16:44

Test fixture comment

249892f

Signed-off-by: Michael Behr <mkbehr@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into cache…

c902e45

…-filter-test-mock-cache Signed-off-by: Michael Behr <mkbehr@google.com>

repokitteh-read-only bot added the waiting label Apr 5, 2022

remove std::move

a3e42a6

Signed-off-by: Michael Behr <mkbehr@google.com>

repokitteh-read-only bot removed the waiting label Apr 6, 2022

zuercher assigned toddmgreer and jmarantz Apr 11, 2022

capoferro previously approved these changes Apr 13, 2022

View reviewed changes

test/extensions/filters/http/cache/cache_filter_test.cc Outdated Show resolved Hide resolved

jmarantz reviewed Apr 18, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 18, 2022

address review

202a0df

Signed-off-by: Michael Behr <mkbehr@google.com>

mkbehr dismissed capoferro’s stale review via 202a0df April 18, 2022 19:38

repokitteh-read-only bot removed the waiting label Apr 18, 2022

jmarantz reviewed Apr 18, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 19, 2022

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 19, 2022

github-actions bot closed this May 26, 2022

Conversation

mkbehr commented Mar 30, 2022

Uh oh!

repokitteh-read-only bot commented Mar 30, 2022

Uh oh!

mkbehr commented Mar 30, 2022

Uh oh!

adisuissa commented Mar 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wrowe commented Apr 5, 2022

Uh oh!

wrowe commented Apr 5, 2022

Uh oh!

wrowe commented Apr 5, 2022

Uh oh!

mkbehr commented Apr 6, 2022

Uh oh!

jmarantz commented Apr 12, 2022

Uh oh!

capoferro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!