From 4191183cac410f19b556451e5edd08cf0b3c41d1 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Thu, 25 Jul 2019 17:03:44 -0700
Subject: [PATCH 01/11] tools: add coverage reporting & enforcement to router
 check

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 .../root/configuration/tools/router_check.rst | 28 +++++++++++++++++--
 docs/root/intro/version_history.rst           |  1 +
 test/tools/router_check/BUILD                 |  4 +++
 test/tools/router_check/coverage.cc           | 22 +++++++++++++++
 test/tools/router_check/coverage.h            | 27 ++++++++++++++++++
 test/tools/router_check/router.cc             | 17 ++++++++---
 test/tools/router_check/router.h              | 12 +++++++-
 test/tools/router_check/router_check.cc       | 10 +++++++
 8 files changed, 113 insertions(+), 8 deletions(-)
 create mode 100644 test/tools/router_check/coverage.cc
 create mode 100644 test/tools/router_check/coverage.h

diff --git a/docs/root/configuration/tools/router_check.rst b/docs/root/configuration/tools/router_check.rst
index fa6dd076eafb..5a596521e95c 100644
--- a/docs/root/configuration/tools/router_check.rst
+++ b/docs/root/configuration/tools/router_check.rst
@@ -3,9 +3,11 @@
 Route table check tool
 ======================
 
-**NOTE: The following configuration is for the route table check tool only and is not part of the Envoy binary.
-The route table check tool is a standalone binary that can be used to verify Envoy's routing for a given configuration
-file.**
+.. note::
+
+  The following configuration is for the route table check tool only and is not part of the Envoy binary.
+  The route table check tool is a standalone binary that can be used to verify Envoy's routing for a given configuration
+  file.
 
 The following specifies input to the route table check tool. The route table check tool checks if
 the route returned by a :ref:`router <envoy_api_msg_RouteConfiguration>` matches what is expected.
@@ -148,3 +150,23 @@ validate
 
     value
       *(required, string)* The value of the header field to match.
+
+Coverage
+--------
+
+The router check tool will report route coverage at the end of a successful test run.
+
+.. code:: bash
+
+  > bazel-bin/test/tools/router_check/router_check_tool --config-path ... --test-path ... --useproto
+  Current route coverage: 0.0744863
+
+This reporting can be leveraged to enforce a minimum coverage percentage by using
+the `-f` or `--fail-under` flag. If coverage falls below this percentage the test
+run will fail.
+
+.. code:: bash
+
+  > bazel-bin/test/tools/router_check/router_check_tool --config-path ... --test-path ... --useproto --fail-under 0.08
+  Current route coverage: 0.0744863
+  Failed to meet coverage requirement: 0.08
diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
index 70e8fe3e8c87..fc29930f0b33 100644
--- a/docs/root/intro/version_history.rst
+++ b/docs/root/intro/version_history.rst
@@ -3,6 +3,7 @@ Version history
 
 1.12.0 (pending)
 ================
+* router check tool: add coverage reporting & enforcement.
 
 1.11.0 (July 11, 2019)
 ======================
diff --git a/test/tools/router_check/BUILD b/test/tools/router_check/BUILD
index ceb012625dd5..c2a581923606 100644
--- a/test/tools/router_check/BUILD
+++ b/test/tools/router_check/BUILD
@@ -19,6 +19,8 @@ envoy_cc_test_binary(
 envoy_cc_test_library(
     name = "router_check_main_lib",
     srcs = [
+        "coverage.cc",
+        "coverage.h",
         "router.cc",
         "router.h",
     ],
@@ -33,6 +35,8 @@ envoy_cc_test_library(
         "//source/common/json:json_loader_lib",
         "//source/common/router:config_lib",
         "//source/common/stats:stats_lib",
+        "//source/extensions/filters/http/buffer:config",
+        "//source/extensions/filters/http/csrf:config",
         "//test/mocks/server:server_mocks",
         "//test/test_common:printers_lib",
         "//test/test_common:utility_lib",
diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
new file mode 100644
index 000000000000..1723f54f46dd
--- /dev/null
+++ b/test/tools/router_check/coverage.cc
@@ -0,0 +1,22 @@
+#include "test/tools/router_check/coverage.h"
+
+#include <algorithm>
+
+#include "envoy/api/v2/core/base.pb.h"
+
+namespace Envoy {
+void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
+  bool seen = std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
+  if (!seen) {
+    seen_routes_.push_back(route);
+  }
+}
+
+double Coverage::report() {
+  int total_route_count = 0;
+  for (const auto& host : route_config_.virtual_hosts()) {
+    total_route_count += host.routes_size();
+  }
+  return static_cast<double>(seen_routes_.size()) / total_route_count;
+}
+} // namespace Envoy
diff --git a/test/tools/router_check/coverage.h b/test/tools/router_check/coverage.h
new file mode 100644
index 000000000000..a07b597dcf11
--- /dev/null
+++ b/test/tools/router_check/coverage.h
@@ -0,0 +1,27 @@
+#pragma once
+
+#include <memory>
+#include <string>
+
+#include "envoy/api/v2/core/base.pb.h"
+#include "envoy/router/router.h"
+
+#include "common/common/empty_string.h"
+#include "common/common/logger.h"
+#include "common/stats/fake_symbol_table_impl.h"
+
+#include "test/mocks/server/mocks.h"
+#include "test/test_common/utility.h"
+
+namespace Envoy {
+class Coverage : Logger::Loggable<Logger::Id::testing> {
+public:
+  Coverage(envoy::api::v2::RouteConfiguration config) : route_config_(config){};
+  void markCovered(const Envoy::Router::RouteEntry* route);
+  double report();
+
+private:
+  std::vector<const Envoy::Router::RouteEntry*> seen_routes_;
+  envoy::api::v2::RouteConfiguration route_config_;
+};
+} // namespace Envoy
diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
index 74b1437fca59..1056ef111586 100644
--- a/test/tools/router_check/router.cc
+++ b/test/tools/router_check/router.cc
@@ -73,16 +73,17 @@ RouterCheckTool RouterCheckTool::create(const std::string& router_config_file) {
   auto factory_context = std::make_unique<NiceMock<Server::Configuration::MockFactoryContext>>();
   auto config = std::make_unique<Router::ConfigImpl>(route_config, *factory_context, false);
 
+  Coverage coverage_report = Coverage(route_config);
   return RouterCheckTool(std::move(factory_context), std::move(config), std::move(stats),
-                         std::move(api));
+                         std::move(api), std::move(coverage_report));
 }
 
 RouterCheckTool::RouterCheckTool(
     std::unique_ptr<NiceMock<Server::Configuration::MockFactoryContext>> factory_context,
     std::unique_ptr<Router::ConfigImpl> config, std::unique_ptr<Stats::IsolatedStoreImpl> stats,
-    Api::ApiPtr api)
+    Api::ApiPtr api, Coverage coverage)
     : factory_context_(std::move(factory_context)), config_(std::move(config)),
-      stats_(std::move(stats)), api_(std::move(api)) {
+      stats_(std::move(stats)), api_(std::move(api)), coverage_(std::move(coverage)) {
   ON_CALL(factory_context_->runtime_loader_.snapshot_,
           featureEnabled(_, testing::An<const envoy::type::FractionalPercent&>(),
                          testing::An<uint64_t>()))
@@ -281,7 +282,11 @@ bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::str
 
     actual = tool_config.headers_->get_(Http::Headers::get().Path);
   }
-  return compareResults(actual, expected, "path_rewrite");
+  bool matches = compareResults(actual, expected, "path_rewrite");
+  if (matches) {
+    coverage_.markCovered(tool_config.route_->routeEntry());
+  }
+  return matches;
 }
 
 bool RouterCheckTool::compareRewritePath(
@@ -415,6 +420,9 @@ Options::Options(int argc, char** argv) {
   TCLAP::CmdLine cmd("router_check_tool", ' ', "none", true);
   TCLAP::SwitchArg is_proto("p", "useproto", "Use Proto test file schema", cmd, false);
   TCLAP::SwitchArg is_detailed("d", "details", "Show detailed test execution results", cmd, false);
+  TCLAP::ValueArg<double> fail_under("f", "fail-under",
+                                     "Fail if test coverage is under a specifed amount", false, 0.0,
+                                     "float", cmd);
   TCLAP::ValueArg<std::string> config_path("c", "config-path", "Path to configuration file.", false,
                                            "", "string", cmd);
   TCLAP::ValueArg<std::string> test_path("t", "test-path", "Path to test file.", false, "",
@@ -430,6 +438,7 @@ Options::Options(int argc, char** argv) {
 
   is_proto_ = is_proto.getValue();
   is_detailed_ = is_detailed.getValue();
+  fail_under_ = fail_under.getValue();
 
   if (is_proto_) {
     config_path_ = config_path.getValue();
diff --git a/test/tools/router_check/router.h b/test/tools/router_check/router.h
index 026bc1343a9a..c11da09f3152 100644
--- a/test/tools/router_check/router.h
+++ b/test/tools/router_check/router.h
@@ -17,6 +17,7 @@
 #include "test/test_common/global.h"
 #include "test/test_common/printers.h"
 #include "test/test_common/utility.h"
+#include "test/tools/router_check/coverage.h"
 #include "test/tools/router_check/json/tool_config_schemas.h"
 #include "test/tools/router_check/validation.pb.h"
 #include "test/tools/router_check/validation.pb.validate.h"
@@ -88,11 +89,13 @@ class RouterCheckTool : Logger::Loggable<Logger::Id::testing> {
    */
   void setShowDetails() { details_ = true; }
 
+  float coverage() { return coverage_.report(); }
+
 private:
   RouterCheckTool(
       std::unique_ptr<NiceMock<Server::Configuration::MockFactoryContext>> factory_context,
       std::unique_ptr<Router::ConfigImpl> config, std::unique_ptr<Stats::IsolatedStoreImpl> stats,
-      Api::ApiPtr api);
+      Api::ApiPtr api, Coverage coverage);
 
   bool compareCluster(ToolConfig& tool_config, const std::string& expected);
   bool compareCluster(ToolConfig& tool_config,
@@ -143,6 +146,7 @@ class RouterCheckTool : Logger::Loggable<Logger::Id::testing> {
   std::unique_ptr<Stats::IsolatedStoreImpl> stats_;
   Api::ApiPtr api_;
   std::string active_runtime;
+  Coverage coverage_;
 };
 
 /**
@@ -172,6 +176,11 @@ class Options {
    */
   const std::string& unlabelledTestPath() const { return unlabelled_test_path_; }
 
+  /**
+   * @return the minimum required percentage of routes coverage.
+   */
+  double failUnder() const { return fail_under_; }
+
   /**
    * @return true if proto schema test is used.
    */
@@ -187,6 +196,7 @@ class Options {
   std::string config_path_;
   std::string unlabelled_test_path_;
   std::string unlabelled_config_path_;
+  float fail_under_;
   bool is_proto_;
   bool is_detailed_;
 };
diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc
index 40f6acec3948..84bc8cb9fe45 100644
--- a/test/tools/router_check/router_check.cc
+++ b/test/tools/router_check/router_check.cc
@@ -7,6 +7,7 @@
 int main(int argc, char* argv[]) {
   Envoy::Options options(argc, argv);
 
+  bool enforceCoverage = options.failUnder() != 0.0;
   try {
     Envoy::RouterCheckTool checktool =
         options.isProto() ? Envoy::RouterCheckTool::create(options.configPath())
@@ -23,6 +24,15 @@ int main(int argc, char* argv[]) {
     if (!is_equal) {
       return EXIT_FAILURE;
     }
+
+    double current_coverage = checktool.coverage();
+    std::cerr << "Current route coverage: " << current_coverage << std::endl;
+    if (enforceCoverage) {
+      if (current_coverage < options.failUnder()) {
+        std::cerr << "Failed to meet coverage requirement: " << options.failUnder() << std::endl;
+        return EXIT_FAILURE;
+      }
+    }
   } catch (const Envoy::EnvoyException& ex) {
     std::cerr << ex.what() << std::endl;
     return EXIT_FAILURE;

From ffe69d914525b6315254ed731571db158b3e6fac Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Thu, 25 Jul 2019 17:34:08 -0700
Subject: [PATCH 02/11] fix spelling in flag description

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/router.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
index 1056ef111586..a4a5d5b29b09 100644
--- a/test/tools/router_check/router.cc
+++ b/test/tools/router_check/router.cc
@@ -421,8 +421,8 @@ Options::Options(int argc, char** argv) {
   TCLAP::SwitchArg is_proto("p", "useproto", "Use Proto test file schema", cmd, false);
   TCLAP::SwitchArg is_detailed("d", "details", "Show detailed test execution results", cmd, false);
   TCLAP::ValueArg<double> fail_under("f", "fail-under",
-                                     "Fail if test coverage is under a specifed amount", false, 0.0,
-                                     "float", cmd);
+                                     "Fail if test coverage is under a specified amount", false,
+                                     0.0, "float", cmd);
   TCLAP::ValueArg<std::string> config_path("c", "config-path", "Path to configuration file.", false,
                                            "", "string", cmd);
   TCLAP::ValueArg<std::string> test_path("t", "test-path", "Path to test file.", false, "",

From 1eb733702ab774f457ee03672d2a2bbbd8fb806c Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Fri, 26 Jul 2019 09:49:29 -0700
Subject: [PATCH 03/11] update total route count variable name

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index 1723f54f46dd..73619c955b53 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -13,10 +13,10 @@ void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
 }
 
 double Coverage::report() {
-  int total_route_count = 0;
+  int size_t = 0;
   for (const auto& host : route_config_.virtual_hosts()) {
-    total_route_count += host.routes_size();
+    size_t += host.routes_size();
   }
-  return static_cast<double>(seen_routes_.size()) / total_route_count;
+  return static_cast<double>(seen_routes_.size()) / size_t;
 }
 } // namespace Envoy

From b8fc7469bbe3f6ebb4c988503074d9efcc4e551e Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Fri, 26 Jul 2019 22:12:12 -0700
Subject: [PATCH 04/11] constants

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc     | 2 +-
 test/tools/router_check/router.cc       | 2 +-
 test/tools/router_check/router_check.cc | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index 73619c955b53..e07c32c27c73 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -6,7 +6,7 @@
 
 namespace Envoy {
 void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
-  bool seen = std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
+  const bool seen = std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
   if (!seen) {
     seen_routes_.push_back(route);
   }
diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
index a4a5d5b29b09..025184ca535d 100644
--- a/test/tools/router_check/router.cc
+++ b/test/tools/router_check/router.cc
@@ -282,7 +282,7 @@ bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::str
 
     actual = tool_config.headers_->get_(Http::Headers::get().Path);
   }
-  bool matches = compareResults(actual, expected, "path_rewrite");
+  const bool matches = compareResults(actual, expected, "path_rewrite");
   if (matches) {
     coverage_.markCovered(tool_config.route_->routeEntry());
   }
diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc
index 84bc8cb9fe45..e57c237299eb 100644
--- a/test/tools/router_check/router_check.cc
+++ b/test/tools/router_check/router_check.cc
@@ -7,7 +7,7 @@
 int main(int argc, char* argv[]) {
   Envoy::Options options(argc, argv);
 
-  bool enforceCoverage = options.failUnder() != 0.0;
+  const bool enforceCoverage = options.failUnder() != 0.0;
   try {
     Envoy::RouterCheckTool checktool =
         options.isProto() ? Envoy::RouterCheckTool::create(options.configPath())
@@ -25,7 +25,7 @@ int main(int argc, char* argv[]) {
       return EXIT_FAILURE;
     }
 
-    double current_coverage = checktool.coverage();
+    const double current_coverage = checktool.coverage();
     std::cerr << "Current route coverage: " << current_coverage << std::endl;
     if (enforceCoverage) {
       if (current_coverage < options.failUnder()) {

From 8954c3c2426bc5d6469b4894595ad904224f568c Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Fri, 26 Jul 2019 22:12:23 -0700
Subject: [PATCH 05/11] fix bug in coverage percentage report

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index e07c32c27c73..1f49f72a33a9 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -17,6 +17,6 @@ double Coverage::report() {
   for (const auto& host : route_config_.virtual_hosts()) {
     size_t += host.routes_size();
   }
-  return static_cast<double>(seen_routes_.size()) / size_t;
+  return 100 * static_cast<double>(seen_routes_.size()) / size_t;
 }
 } // namespace Envoy

From 2999c10de2e98b71c50af12447b8480a50363ea0 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Fri, 26 Jul 2019 22:14:37 -0700
Subject: [PATCH 06/11] add percentage sign to output

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc     | 3 ++-
 test/tools/router_check/router_check.cc | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index 1f49f72a33a9..b71819ed66f0 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -6,7 +6,8 @@
 
 namespace Envoy {
 void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
-  const bool seen = std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
+  const bool seen =
+      std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
   if (!seen) {
     seen_routes_.push_back(route);
   }
diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc
index e57c237299eb..461e7ffe4bc1 100644
--- a/test/tools/router_check/router_check.cc
+++ b/test/tools/router_check/router_check.cc
@@ -26,10 +26,11 @@ int main(int argc, char* argv[]) {
     }
 
     const double current_coverage = checktool.coverage();
-    std::cerr << "Current route coverage: " << current_coverage << std::endl;
+    std::cerr << "Current route coverage: " << current_coverage << "%" << std::endl;
     if (enforceCoverage) {
       if (current_coverage < options.failUnder()) {
-        std::cerr << "Failed to meet coverage requirement: " << options.failUnder() << std::endl;
+        std::cerr << "Failed to meet coverage requirement: " << options.failUnder() << "%"
+                  << std::endl;
         return EXIT_FAILURE;
       }
     }

From 2a1bbac4b4e820f9b134445decbc4867b749a0d9 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Sat, 27 Jul 2019 00:40:13 -0700
Subject: [PATCH 07/11] remove http filters from build

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/BUILD | 2 --
 1 file changed, 2 deletions(-)

diff --git a/test/tools/router_check/BUILD b/test/tools/router_check/BUILD
index c2a581923606..c4a1a2facf4b 100644
--- a/test/tools/router_check/BUILD
+++ b/test/tools/router_check/BUILD
@@ -35,8 +35,6 @@ envoy_cc_test_library(
         "//source/common/json:json_loader_lib",
         "//source/common/router:config_lib",
         "//source/common/stats:stats_lib",
-        "//source/extensions/filters/http/buffer:config",
-        "//source/extensions/filters/http/csrf:config",
         "//test/mocks/server:server_mocks",
         "//test/test_common:printers_lib",
         "//test/test_common:utility_lib",

From 10941896d8143fa179383ff6e386bc3f5e1472f4 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Tue, 30 Jul 2019 22:16:36 -0700
Subject: [PATCH 08/11] update based on feedback

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc         | 10 ++++------
 test/tools/router_check/coverage.h          | 11 +----------
 test/tools/router_check/router.cc           |  3 +--
 test/tools/router_check/test/route_tests.sh | 22 +++++++++++++++++----
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index b71819ed66f0..114c506be437 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -6,18 +6,16 @@
 
 namespace Envoy {
 void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
-  const bool seen =
-      std::find(seen_routes_.begin(), seen_routes_.end(), route) != seen_routes_.end();
-  if (!seen) {
+  if (std::find(seen_routes_.begin(), seen_routes_.end(), route) == seen_routes_.end()) {
     seen_routes_.push_back(route);
   }
 }
 
 double Coverage::report() {
-  int size_t = 0;
+  uint64_t num_routes = 0;
   for (const auto& host : route_config_.virtual_hosts()) {
-    size_t += host.routes_size();
+    num_routes += host.routes_size();
   }
-  return 100 * static_cast<double>(seen_routes_.size()) / size_t;
+  return 100 * static_cast<double>(seen_routes_.size()) / num_routes;
 }
 } // namespace Envoy
diff --git a/test/tools/router_check/coverage.h b/test/tools/router_check/coverage.h
index a07b597dcf11..1c6eed0c2c16 100644
--- a/test/tools/router_check/coverage.h
+++ b/test/tools/router_check/coverage.h
@@ -1,17 +1,8 @@
 #pragma once
 
-#include <memory>
-#include <string>
-
-#include "envoy/api/v2/core/base.pb.h"
 #include "envoy/router/router.h"
 
-#include "common/common/empty_string.h"
-#include "common/common/logger.h"
-#include "common/stats/fake_symbol_table_impl.h"
-
 #include "test/mocks/server/mocks.h"
-#include "test/test_common/utility.h"
 
 namespace Envoy {
 class Coverage : Logger::Loggable<Logger::Id::testing> {
@@ -22,6 +13,6 @@ class Coverage : Logger::Loggable<Logger::Id::testing> {
 
 private:
   std::vector<const Envoy::Router::RouteEntry*> seen_routes_;
-  envoy::api::v2::RouteConfiguration route_config_;
+  const envoy::api::v2::RouteConfiguration route_config_;
 };
 } // namespace Envoy
diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
index 025184ca535d..05c25863a053 100644
--- a/test/tools/router_check/router.cc
+++ b/test/tools/router_check/router.cc
@@ -73,9 +73,8 @@ RouterCheckTool RouterCheckTool::create(const std::string& router_config_file) {
   auto factory_context = std::make_unique<NiceMock<Server::Configuration::MockFactoryContext>>();
   auto config = std::make_unique<Router::ConfigImpl>(route_config, *factory_context, false);
 
-  Coverage coverage_report = Coverage(route_config);
   return RouterCheckTool(std::move(factory_context), std::move(config), std::move(stats),
-                         std::move(api), std::move(coverage_report));
+                         std::move(api), Coverage(route_config));
 }
 
 RouterCheckTool::RouterCheckTool(
diff --git a/test/tools/router_check/test/route_tests.sh b/test/tools/router_check/test/route_tests.sh
index 366c23a73333..cba937d3f7e3 100755
--- a/test/tools/router_check/test/route_tests.sh
+++ b/test/tools/router_check/test/route_tests.sh
@@ -16,6 +16,20 @@ do
   TEST_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/${t}.yaml" "${PATH_CONFIG}/${t}.golden.json" "--details")
 done
 
+# Testing coverage flag passes
+COVERAGE_CMD="${PATH_BIN} ${PATH_CONFIG}/Redirect.yaml ${PATH_CONFIG}/Redirect.golden.json --details -f "
+TEST_OUTPUT=$($COVERAGE_CMD "1.0")
+COVERAGE_OUTPUT=$($COVERAGE_CMD "1.0" 2>&1) || echo "${COVERAGE_OUTPUT:-no-output}"
+if [[ "${COVERAGE_OUTPUT}" != *"Current route coverage: "* ]] ; then
+  exit 1
+fi
+
+# Testing coverage flag fails
+COVERAGE_OUTPUT=$($COVERAGE_CMD "100" 2>&1) || echo "${COVERAGE_OUTPUT:-no-output}"
+if [[ "${COVERAGE_OUTPUT}" != *"Failed to meet coverage requirement: 100%"* ]] ; then
+  exit 1
+fi
+
 # Testing expected matches using --useproto
 # --useproto needs the test schema as a validation.proto message.
 TESTS+=("Runtime")
@@ -31,17 +45,17 @@ TEST_OUTPUT=$("${PATH_BIN}" "-c" "${PATH_CONFIG}/Weighted.yaml" "-t" "${PATH_CON
 TEST_OUTPUT=$("${PATH_BIN}" "-c" "${PATH_CONFIG}/Weighted.yaml" "-t" "${PATH_CONFIG}/Weighted.golden.proto.pb_text" "--details" "--useproto")
 
 # Bad config file
-echo testing bad config output
+echo "testing bad config output"
 BAD_CONFIG_OUTPUT=$(("${PATH_BIN}" "${PATH_CONFIG}/Redirect.golden.json" "${PATH_CONFIG}/TestRoutes.yaml") 2>&1) ||
-  echo ${BAD_CONFIG_OUTPUT:-no-output}
+  echo "${BAD_CONFIG_OUTPUT:-no-output}"
 if [[ "${BAD_CONFIG_OUTPUT}" != *"Unable to parse"* ]]; then
   exit 1
 fi
 
 # Failure test case
-echo testing failure test case
+echo "testing failure test case"
 FAILURE_OUTPUT=$("${PATH_BIN}" "${PATH_CONFIG}/TestRoutes.yaml" "${PATH_CONFIG}/Weighted.golden.json" "--details" 2>&1) ||
-  echo ${FAILURE_OUTPUT:-no-output}
+  echo "${FAILURE_OUTPUT:-no-output}"
 if [[ "${FAILURE_OUTPUT}" != *"expected: [cluster1], actual: [instant-server], test type: cluster_name"* ]]; then
   exit 1
 fi

From 6f5e230541f79ebdff4d9d3ed39c4a3d657e8356 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Wed, 31 Jul 2019 10:40:04 -0700
Subject: [PATCH 09/11] pass by reference

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc | 6 +++---
 test/tools/router_check/coverage.h  | 2 +-
 test/tools/router_check/router.cc   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index 114c506be437..ab46ae4351bf 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -5,9 +5,9 @@
 #include "envoy/api/v2/core/base.pb.h"
 
 namespace Envoy {
-void Coverage::markCovered(const Envoy::Router::RouteEntry* route) {
-  if (std::find(seen_routes_.begin(), seen_routes_.end(), route) == seen_routes_.end()) {
-    seen_routes_.push_back(route);
+void Coverage::markCovered(const Envoy::Router::RouteEntry& route) {
+  if (std::find(seen_routes_.begin(), seen_routes_.end(), &route) == seen_routes_.end()) {
+    seen_routes_.push_back(&route);
   }
 }
 
diff --git a/test/tools/router_check/coverage.h b/test/tools/router_check/coverage.h
index 1c6eed0c2c16..778906a89fca 100644
--- a/test/tools/router_check/coverage.h
+++ b/test/tools/router_check/coverage.h
@@ -8,7 +8,7 @@ namespace Envoy {
 class Coverage : Logger::Loggable<Logger::Id::testing> {
 public:
   Coverage(envoy::api::v2::RouteConfiguration config) : route_config_(config){};
-  void markCovered(const Envoy::Router::RouteEntry* route);
+  void markCovered(const Envoy::Router::RouteEntry& route);
   double report();
 
 private:
diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc
index 05c25863a053..70ed8c6fbd19 100644
--- a/test/tools/router_check/router.cc
+++ b/test/tools/router_check/router.cc
@@ -283,7 +283,7 @@ bool RouterCheckTool::compareRewritePath(ToolConfig& tool_config, const std::str
   }
   const bool matches = compareResults(actual, expected, "path_rewrite");
   if (matches) {
-    coverage_.markCovered(tool_config.route_->routeEntry());
+    coverage_.markCovered(*tool_config.route_->routeEntry());
   }
   return matches;
 }

From d5ff7af92d40343719522b93ca11ef8ee1897271 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Wed, 31 Jul 2019 14:10:52 -0700
Subject: [PATCH 10/11] add note about avoiding duplicate coverage

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/coverage.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/tools/router_check/coverage.cc b/test/tools/router_check/coverage.cc
index ab46ae4351bf..5f8c51a51351 100644
--- a/test/tools/router_check/coverage.cc
+++ b/test/tools/router_check/coverage.cc
@@ -6,6 +6,8 @@
 
 namespace Envoy {
 void Coverage::markCovered(const Envoy::Router::RouteEntry& route) {
+  // n.b. If we reach the end of the seen routes without finding the specified
+  // route we add it as seen, otherwise it's a duplicate.
   if (std::find(seen_routes_.begin(), seen_routes_.end(), &route) == seen_routes_.end()) {
     seen_routes_.push_back(&route);
   }

From 7c5c4a45a8beba01317018d8eacaca499c4d9cf6 Mon Sep 17 00:00:00 2001
From: Derek Schaller <d_a_schaller@yahoo.com>
Date: Wed, 31 Jul 2019 16:36:01 -0700
Subject: [PATCH 11/11] fix nit

Signed-off-by: Derek Schaller <d_a_schaller@yahoo.com>
---
 test/tools/router_check/router_check.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/tools/router_check/router_check.cc b/test/tools/router_check/router_check.cc
index 461e7ffe4bc1..d7b27d99f3a3 100644
--- a/test/tools/router_check/router_check.cc
+++ b/test/tools/router_check/router_check.cc
@@ -7,7 +7,7 @@
 int main(int argc, char* argv[]) {
   Envoy::Options options(argc, argv);
 
-  const bool enforceCoverage = options.failUnder() != 0.0;
+  const bool enforce_coverage = options.failUnder() != 0.0;
   try {
     Envoy::RouterCheckTool checktool =
         options.isProto() ? Envoy::RouterCheckTool::create(options.configPath())
@@ -27,7 +27,7 @@ int main(int argc, char* argv[]) {
 
     const double current_coverage = checktool.coverage();
     std::cerr << "Current route coverage: " << current_coverage << "%" << std::endl;
-    if (enforceCoverage) {
+    if (enforce_coverage) {
       if (current_coverage < options.failUnder()) {
         std::cerr << "Failed to meet coverage requirement: " << options.failUnder() << "%"
                   << std::endl;