Add unit tests for environment with fake HTTP server. #139

davidbtucker · 2018-05-23T18:51:30Z

No description provided.

igorpeshansky · 2018-05-25T02:35:39Z

src/environment.cc

@@ -83,7 +80,7 @@ Environment::Environment(const Configuration& config)
 std::string Environment::GetMetadataString(const std::string& path) const {
  http::client::options options;
  http::client client(options.timeout(2));
-  http::client::request request(kGceMetadataServerAddress + path);
+  http::client::request request(config_.GceMetadataServerAddress() + path);


I'd rather not make this a configuration parameter. There is no reason for anything but our tests to override this. You could instead make this an instance field of the Environment object, and either add a private constructor that injects it, or a private test-only setter.

+1 to not making this a config parameter.

igorpeshansky · 2018-05-25T02:46:25Z

test/environment_unittest.cc

+TEST_F(EnvironmentTest, GetMetadataStringWithFakeServer) {
+  // Start a server in a separate thread, and allow it to choose an
+  // available port.
+  FakeHandler handler(std::map<std::string, std::string>({{"/a/b/c", "hello"}}));


It might be nicer to follow the style of Python testing and add a SetResponse(path, response) method (maybe even SetResponse(path, code, response))... Then you can fold the handler and the options object into the FakeServer implementation, rather than expose them to clients.

Done (did not include code yet).

igorpeshansky · 2018-05-25T02:48:14Z

test/environment_unittest.cc

+  FakeServer::options options(handler);
+  FakeServer server(options.address("127.0.0.1").port(""));
+  server.listen();
+  std::thread t1([&server] { server.run(); });


Your fake server is always single-threaded, so you can fold the thread into the server object, regardless (calling join() in the destructor).

igorpeshansky · 2018-05-25T03:57:15Z

test/environment_unittest.cc

+    "{\"client_email\":\"[email protected]\",\"private_key\":\"some_key\"}");
+  std::string cfg;
+  Configuration config(std::istringstream(
+      "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n"


Why is this necessary for testing a fake server?

Aha, it's not! Removed.

igorpeshansky · 2018-05-25T04:07:28Z

test/environment_unittest.cc

+  std::string cfg;
+  Configuration config(std::istringstream(
+      "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n"
+      "GceMetadataServerAddress: 'http://invalidaddress'\n"


This is fragile, and breaks in environments where an ISP intercepts invalid lookups:

environment_unittest.cc:171: Failure Expected equality of these values: "" environment.GetMetadataString("/a/b/c") Which is: "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Transitional//EN\" \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd\"><html><head><meta http-equiv=\"refresh\" content=\"0;url=http://searchassist.verizon.com/main?ParticipantID=euekiz39ksg8nwp7iqj2fp5wzfwi5q76&FailedURI=http%3A%2F%2Finvalidaddress%2Fa%2Fb%2Fc&FailureMode=1&Implementation=&AddInType=4&Version=pywr1.0&ClientLocation=us\"/><script type=\"text/javascript\">url=\"http://searchassist.verizon.com/main?ParticipantID=euekiz39ksg8nwp7iqj2fp5wzfwi5q76&FailedURI=http%3A%2F%2Finvalidaddress%2Fa%2Fb%2Fc&FailureMode=1&Implementation=&AddInType=4&Version=pywr1.0&ClientLocation=us\";if(top.location!=location){var w=window,d=document,e=d.documentElement,b=d.body,x=w.innerWidth||e.clientWidth||b.clientWidth,y=w.innerHeight||e.clientHeight||b.clientHeight;url+=\"&w=\"+x+\"&h=\"+y;}window.location.replace(url);</script></head><body></body></html>"

We should instead make the fake server throw boost::system::system_error on simulated resolution failures.

I removed this test altogether, as the other test already exercises this catch clause.

davidbtucker

Thanks PTAL

davidbtucker · 2018-05-29T21:39:34Z

src/environment.cc

@@ -83,7 +80,7 @@ Environment::Environment(const Configuration& config)
 std::string Environment::GetMetadataString(const std::string& path) const {
  http::client::options options;
  http::client client(options.timeout(2));
-  http::client::request request(kGceMetadataServerAddress + path);
+  http::client::request request(config_.GceMetadataServerAddress() + path);


davidbtucker · 2018-05-29T21:40:14Z

test/environment_unittest.cc

+TEST_F(EnvironmentTest, GetMetadataStringWithFakeServer) {
+  // Start a server in a separate thread, and allow it to choose an
+  // available port.
+  FakeHandler handler(std::map<std::string, std::string>({{"/a/b/c", "hello"}}));


Done (did not include code yet).

davidbtucker · 2018-05-29T21:40:42Z

test/environment_unittest.cc

+  FakeServer::options options(handler);
+  FakeServer server(options.address("127.0.0.1").port(""));
+  server.listen();
+  std::thread t1([&server] { server.run(); });


davidbtucker · 2018-05-29T21:41:09Z

test/environment_unittest.cc

+    "{\"client_email\":\"[email protected]\",\"private_key\":\"some_key\"}");
+  std::string cfg;
+  Configuration config(std::istringstream(
+      "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n"


Aha, it's not! Removed.

davidbtucker · 2018-05-29T21:42:04Z

test/environment_unittest.cc

+  std::string cfg;
+  Configuration config(std::istringstream(
+      "CredentialsFile: '" + credentials_file.FullPath().native() + "'\n"
+      "GceMetadataServerAddress: 'http://invalidaddress'\n"


I removed this test altogether, as the other test already exercises this catch clause.

supriyagarg

LGTM

igorpeshansky

Some naming/location nits.

igorpeshansky · 2018-05-25T20:22:22Z

test/environment_unittest.cc

+    } else {
+      // Note: We have to set headers; otherwise, an exception is thrown.
+      connection->set_status(FakeServer::connection::not_found);
+      connection->set_headers(std::map<std::string, std::string>({}));


It's unfortunate that this bit is necessary, but we can deal with this later.

igorpeshansky · 2018-05-30T16:19:43Z

src/environment.cc

@@ -78,12 +78,14 @@ constexpr const char kGceInstanceResourceType[] = "gce_instance";
 }

 Environment::Environment(const Configuration& config)
-    : config_(config), application_default_credentials_read_(false) {}
+    : config_(config),
+      gce_metadata_server_address_(kGceMetadataServerAddress),


Let's just call it metadata_server_url_.

igorpeshansky · 2018-05-30T16:20:55Z

src/environment.h

  const Configuration& config_;

+  std::string gce_metadata_server_address_;


igorpeshansky · 2018-05-30T16:21:21Z

src/environment.h

  const Configuration& config_;

+  std::string gce_metadata_server_address_;


This is technically cached data as well. Let's put this just before application_default_credentials_read_.

igorpeshansky · 2018-05-30T16:22:26Z

src/environment.h

@@ -48,8 +48,13 @@ class Environment {

  void ReadApplicationDefaultCredentials() const;

+  // Used to override server address in tests.
+  void SetGceMetadataServerAddress(const std::string& address);


SetMetadataServerUrlForTest, and you can get rid of the comment.

igorpeshansky · 2018-05-30T16:32:03Z

test/environment_unittest.cc

+class FakeServerThread {
+ public:
+  FakeServerThread()
+    : server_(Server::options(handler_).address("127.0.0.1").port("")) {


The "empty port" behavior seems to be undocumented. Sigh.

Acknowledged.

igorpeshansky · 2018-05-30T16:32:57Z

test/environment_unittest.cc

+    handler_.path_responses[path] = response;
+  }
+
+  std::string HostPort() {


Just curious, why HostPort and not GetUrl?

Changed to GetUrl.

igorpeshansky · 2018-05-30T16:33:45Z

test/environment_unittest.cc

+        connection->set_headers(std::map<std::string, std::string>());
+      }
+    }
+    std::map<std::string, std::string> path_responses;


Should this map live in the FakeServer class instead?

I think I'd have to pass in a FakeServer reference to the Handler, which seems more complicated.

You could also just pass a const reference to the map... But it's also fine the way it is.

igorpeshansky · 2018-05-30T16:42:09Z

test/environment_unittest.cc

+
+  Handler handler_;
+  Server server_;
+  std::thread thread_;


server_thread_?

igorpeshansky · 2018-05-30T16:43:19Z

test/environment_unittest.cc

+
+// Starts a server in a separate thread, allowing it to choose an
+// available port.
+class FakeServerThread {


Can we pull this out to a separate header/.cc file now?

davidbtucker

PTAL, thanks!

davidbtucker · 2018-05-30T18:49:56Z

src/environment.cc

@@ -78,12 +78,14 @@ constexpr const char kGceInstanceResourceType[] = "gce_instance";
 }

 Environment::Environment(const Configuration& config)
-    : config_(config), application_default_credentials_read_(false) {}
+    : config_(config),
+      gce_metadata_server_address_(kGceMetadataServerAddress),


davidbtucker · 2018-05-30T18:50:07Z

src/environment.h

@@ -48,8 +48,13 @@ class Environment {

  void ReadApplicationDefaultCredentials() const;

+  // Used to override server address in tests.
+  void SetGceMetadataServerAddress(const std::string& address);


davidbtucker · 2018-05-30T18:52:40Z

src/environment.h

  const Configuration& config_;

+  std::string gce_metadata_server_address_;


davidbtucker · 2018-05-30T18:52:44Z

src/environment.h

  const Configuration& config_;

+  std::string gce_metadata_server_address_;


davidbtucker · 2018-05-30T18:52:55Z

test/environment_unittest.cc

@@ -12,9 +13,15 @@ class EnvironmentTest : public ::testing::Test {
  static void ReadApplicationDefaultCredentials(const Environment& environment) {
    environment.ReadApplicationDefaultCredentials();
  }
+
+  static void SetGceMetadataServerAddress(Environment* environment,


davidbtucker · 2018-05-30T18:54:27Z

test/environment_unittest.cc

+
+  Handler handler_;
+  Server server_;
+  std::thread thread_;


davidbtucker · 2018-05-30T18:55:35Z

test/environment_unittest.cc

+
+// Starts a server in a separate thread, allowing it to choose an
+// available port.
+class FakeServerThread {


davidbtucker · 2018-05-30T18:58:32Z

test/environment_unittest.cc

+
+// Starts a server in a separate thread, allowing it to choose an
+// available port.
+class FakeServerThread {


davidbtucker · 2018-05-30T19:01:29Z

test/environment_unittest.cc

+        connection->set_headers(std::map<std::string, std::string>());
+      }
+    }
+    std::map<std::string, std::string> path_responses;


I think I'd have to pass in a FakeServer reference to the Handler, which seems more complicated.

davidbtucker · 2018-05-30T19:02:00Z

src/environment.h

@@ -48,8 +48,13 @@ class Environment {

  void ReadApplicationDefaultCredentials() const;

+  // Used to override server address in tests.
+  void SetGceMetadataServerAddress(const std::string& address);


igorpeshansky · 2018-05-30T19:37:34Z

test/environment_unittest.cc

+ public:
+  FakeServerThread()
+    : server_(Server::options(handler_).address("127.0.0.1").port("")) {
+    server_.listen();


Cool, can you please add a comment saying exactly that?

igorpeshansky · 2018-05-30T19:38:21Z

test/environment_unittest.cc

+        connection->set_headers(std::map<std::string, std::string>());
+      }
+    }
+    std::map<std::string, std::string> path_responses;


You could also just pass a const reference to the map... But it's also fine the way it is.

igorpeshansky · 2018-05-30T19:42:16Z

src/environment.h

@@ -48,6 +48,10 @@ class Environment {

  void ReadApplicationDefaultCredentials() const;

+  void SetMetadataServerUrlForTest(const std::string& url) {


You could add a comment saying that the url must end in a '/'...

igorpeshansky · 2018-05-30T19:45:37Z

test/environment_unittest.cc

@@ -98,4 +105,17 @@ TEST_F(EnvironmentTest, ReadApplicationDefaultCredentialsCaches) {
  );
  EXPECT_EQ("some_key", environment.CredentialsPrivateKey());
 }
+
+TEST_F(EnvironmentTest, GetMetadataStringWithFakeServer) {


Missed this one before... I'd just call it GetMetadataString — FakeServer is an implementation detail.

igorpeshansky · 2018-05-30T19:46:31Z

test/fake_http_server.cc

+namespace google {
+namespace testing {
+
+FakeServer::FakeServer()


How about a comment along the lines of "an empty port selects a random available port (undocumented)"?

This isn't a class comment — let's move it to where the empty port is passed in.

igorpeshansky · 2018-05-30T20:32:28Z

test/fake_http_server.cc

+}
+
+std::string FakeServer::GetUrl() {
+  return std::string("http://") + server_.address() + ":" + server_.port() + "/";


[Optional]

network::uri_builder builder; builder.scheme("http").host(server_.address()).port(server_.port()).path("/"); return builder.uri().to_string();

igorpeshansky · 2018-05-30T20:33:04Z

test/fake_http_server.cc

+  handler_.path_responses[path] = response;
+}
+
+void FakeServer::Handler::operator() (Server::request const &request,


No space after operator().

igorpeshansky · 2018-05-30T20:33:18Z

test/fake_http_server.h

+
+  // Handler that maps paths to response strings.
+  struct Handler {
+    void operator() (Server::request const &request,


No space after operator().

igorpeshansky

One remaining comment.

igorpeshansky · 2018-06-02T21:33:58Z

test/fake_http_server.cc

+namespace google {
+namespace testing {
+
+FakeServer::FakeServer()


This isn't a class comment — let's move it to where the empty port is passed in.

bmoyles0117 · 2018-06-13T15:20:47Z

src/environment.h

@@ -48,6 +48,11 @@ class Environment {

  void ReadApplicationDefaultCredentials() const;

+  // The url must end in a '/'.
+  void SetMetadataServerUrlForTest(const std::string& url) {


Why is this only "ForTest"? It seems like a reasonable enough setter function to remove that suffix.

Just to make it clear that the method only exists because we need to override the URL in test code. If we ever need to use it in production code, we can rename the method.

bmoyles0117

LGTM

igorpeshansky

LGTM

Add unit tests for environment with fake http server.

90bd2ce

davidbtucker requested review from igorpeshansky and supriyagarg May 23, 2018 18:51

igorpeshansky suggested changes May 25, 2018

View reviewed changes

Addressed feedback.

b686734

davidbtucker commented May 29, 2018

View reviewed changes

Fix slashes in tests.

3d7c1c4

supriyagarg approved these changes May 30, 2018

View reviewed changes

igorpeshansky suggested changes May 30, 2018

View reviewed changes

davidbtucker added 2 commits May 30, 2018 14:49

Addressed more feedback.

ff6fe28

A few comments I missed.

639adf7

davidbtucker commented May 30, 2018

View reviewed changes

igorpeshansky suggested changes May 30, 2018

View reviewed changes

More feedback.

2b5db18

igorpeshansky suggested changes Jun 2, 2018

View reviewed changes

Move comment.

a86a373

bmoyles0117 suggested changes Jun 13, 2018

View reviewed changes

bmoyles0117 approved these changes Jun 29, 2018

View reviewed changes

igorpeshansky approved these changes Jul 17, 2018

View reviewed changes

davidbtucker merged commit 8ec2e47 into master Jul 17, 2018

davidbtucker deleted the davidbtucker-http-mocks branch July 17, 2018 20:34

		const Configuration& config_;

		std::string gce_metadata_server_address_;

		@@ -48,6 +48,10 @@ class Environment {

		void ReadApplicationDefaultCredentials() const;

		void SetMetadataServerUrlForTest(const std::string& url) {

Add unit tests for environment with fake HTTP server. #139

Add unit tests for environment with fake HTTP server. #139

Conversation

davidbtucker commented May 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supriyagarg left a comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker May 30, 2018 • edited by igorpeshansky Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker May 30, 2018 • edited by igorpeshansky Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker May 30, 2018 • edited by igorpeshansky Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbtucker May 30, 2018 •

edited by igorpeshansky

Loading

davidbtucker May 30, 2018 •

edited by igorpeshansky

Loading

davidbtucker May 30, 2018 •

edited by igorpeshansky

Loading