From 9e8dd3490378c9f6f4ec5717457c01b24ba35aa2 Mon Sep 17 00:00:00 2001 From: Ben Spencer Date: Sat, 20 Jun 2020 14:02:34 +0100 Subject: [PATCH 1/2] Tidy up copy-pasted code in integration tests. --- .../integration/CustomTypesSpec.scala | 113 ++++++++---------- .../integration/IntegrationSpec.scala | 47 ++++---- .../postgres/integration/NumericSpec.scala | 23 +--- .../integration/TransactionSpec.scala | 15 +-- 4 files changed, 82 insertions(+), 116 deletions(-) diff --git a/src/test/scala/com/twitter/finagle/postgres/integration/CustomTypesSpec.scala b/src/test/scala/com/twitter/finagle/postgres/integration/CustomTypesSpec.scala index 37d481cb..bc39d795 100644 --- a/src/test/scala/com/twitter/finagle/postgres/integration/CustomTypesSpec.scala +++ b/src/test/scala/com/twitter/finagle/postgres/integration/CustomTypesSpec.scala @@ -42,72 +42,63 @@ class CustomTypesSpec extends Spec with ScalaCheckDrivenPropertyChecks { } } - for { - binaryResults <- Seq(false, true) - binaryParams <- Seq(false, true) - hostPort <- sys.env.get("PG_HOST_PORT") - user <- sys.env.get("PG_USER") - password = sys.env.get("PG_PASSWORD") - dbname <- sys.env.get("PG_DBNAME") - useSsl = sys.env.getOrElse("USE_PG_SSL", "0") == "1" - } yield { + IntegrationSpec.clientBuilder().foreach { clientBuilder => + for { + binaryResults <- Seq(false, true) + binaryParams <- Seq(false, true) + } yield { + implicit val client = clientBuilder + .withBinaryParams(binaryParams) + .withBinaryResults(binaryResults) + .withSessionPool.maxSize(1) + .newRichClient() - implicit val client = Postgres.Client() - .database(dbname) - .withCredentials(user, password) - .withBinaryParams(binaryParams) - .withBinaryResults(binaryResults) - .conditionally(useSsl, _.withTransport.tlsWithoutValidation) - .withSessionPool.maxSize(1) - .newRichClient(hostPort) + val mode = if(binaryResults) "binary mode" else "text mode" + val paramsMode = if(binaryParams) + "binary" + else + "text" + implicit val useBinaryParams = binaryParams - val mode = if(binaryResults) "binary mode" else "text mode" - val paramsMode = if(binaryParams) - "binary" - else - "text" - - implicit val useBinaryParams = binaryParams - - s"A $mode postgres client with $paramsMode params" should { - "retrieve the available types from the remote DB" in { - val types = Await.result(client.typeMap()) - assert(types.nonEmpty) - assert(types != PostgresClient.defaultTypes) + s"A $mode postgres client with $paramsMode params" should { + "retrieve the available types from the remote DB" in { + val types = Await.result(client.typeMap()) + assert(types.nonEmpty) + assert(types != PostgresClient.defaultTypes) + } } - } - s"Retrieved type map decoders for $mode client with $paramsMode params" must { - "parse varchars" in test(ValueDecoder.string)("varchar") - "parse text" in test(ValueDecoder.string)("text") - "parse booleans" in test(ValueDecoder.boolean)("boolean") - "parse shorts" in test(ValueDecoder.int2)("int2") - "parse ints" in test(ValueDecoder.int4)("int4") - "parse longs" in test(ValueDecoder.int8)("int8") - //precision seems to be an issue when postgres parses text floats - "parse floats" in test(ValueDecoder.float4)("numeric::float4") - "parse doubles" in test(ValueDecoder.float8)("numeric::float8") - "parse numerics" in test(ValueDecoder.bigDecimal)("numeric") - "parse timestamps" in test(ValueDecoder.localDateTime)( - "timestamp", - (a, b) => a.getLong(ChronoField.MICRO_OF_DAY) == b.getLong(ChronoField.MICRO_OF_DAY) - ) - "parse timestamps with time zone" in test(ValueDecoder.zonedDateTime)( - "timestamptz", - (a, b) => - // when reading the value, the timezone may have changed: - // the binary protocol does not include timezone information (everything is in UTC) - // the text protocol returns in the server's timezone which may be different than the supplied tz. - // so we convert the input value to the read value's timezone and then compare them - a.withZoneSameInstant(b.getOffset) == b - ) - "parse times" in test(ValueDecoder.localTime)("time") - "parse times with timezone" in test(ValueDecoder.offsetTime)("timetz") - "parse intervals" in test(ValueDecoder.interval)("interval") - "parse uuids" in test(ValueDecoder.uuid)("uuid") - "parse hstore maps" in test(ValueDecoder.hstoreMap)("hstore") + s"Retrieved type map decoders for $mode client with $paramsMode params" must { + "parse varchars" in test(ValueDecoder.string)("varchar") + "parse text" in test(ValueDecoder.string)("text") + "parse booleans" in test(ValueDecoder.boolean)("boolean") + "parse shorts" in test(ValueDecoder.int2)("int2") + "parse ints" in test(ValueDecoder.int4)("int4") + "parse longs" in test(ValueDecoder.int8)("int8") + //precision seems to be an issue when postgres parses text floats + "parse floats" in test(ValueDecoder.float4)("numeric::float4") + "parse doubles" in test(ValueDecoder.float8)("numeric::float8") + "parse numerics" in test(ValueDecoder.bigDecimal)("numeric") + "parse timestamps" in test(ValueDecoder.localDateTime)( + "timestamp", + (a, b) => a.getLong(ChronoField.MICRO_OF_DAY) == b.getLong(ChronoField.MICRO_OF_DAY) + ) + "parse timestamps with time zone" in test(ValueDecoder.zonedDateTime)( + "timestamptz", + (a, b) => + // when reading the value, the timezone may have changed: + // the binary protocol does not include timezone information (everything is in UTC) + // the text protocol returns in the server's timezone which may be different than the supplied tz. + // so we convert the input value to the read value's timezone and then compare them + a.withZoneSameInstant(b.getOffset) == b + ) + "parse times" in test(ValueDecoder.localTime)("time") + "parse times with timezone" in test(ValueDecoder.offsetTime)("timetz") + "parse intervals" in test(ValueDecoder.interval)("interval") + "parse uuids" in test(ValueDecoder.uuid)("uuid") + "parse hstore maps" in test(ValueDecoder.hstoreMap)("hstore") + } } - } } diff --git a/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala b/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala index 6f68f816..3e432a41 100644 --- a/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala +++ b/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala @@ -14,6 +14,24 @@ import com.twitter.util.Future object IntegrationSpec { val pgTestTable = "finagle_test" + + def clientBuilder(): Option[Postgres.Client] = (for { + hostPort <- sys.env.get("PG_HOST_PORT") + user <- sys.env.get("PG_USER") + password = sys.env.get("PG_PASSWORD") + dbname <- sys.env.get("PG_DBNAME") + useSsl = sys.env.getOrElse("USE_PG_SSL", "0") == "1" + sslHost = sys.env.get("PG_SSL_HOST") + } yield { + Postgres.Client() + .withCredentials(user, password) + .database(dbname) + .conditionally(useSsl, c => sslHost.fold(c.withTransport.tls)(c.withTransport.tls(_))) + .dest(hostPort) + }).orElse { + println("WARNING: Skipping integration tests due to missing environment variables, see IntegrationSpec.scala for details") + None + } } /* @@ -30,26 +48,14 @@ object IntegrationSpec { * */ class IntegrationSpec extends Spec { - - for { - hostPort <- sys.env.get("PG_HOST_PORT") - user <- sys.env.get("PG_USER") - password = sys.env.get("PG_PASSWORD") - dbname <- sys.env.get("PG_DBNAME") - useSsl = sys.env.getOrElse("USE_PG_SSL", "0") == "1" - sslHost = sys.env.get("PG_SSL_HOST") - } yield { - + IntegrationSpec.clientBuilder().foreach { clientBuilder => val queryTimeout = Duration.fromSeconds(2) def getClient: PostgresClientImpl = { - val client = Postgres.Client() - .withCredentials(user, password) - .database(dbname) + val client = clientBuilder .withSessionPool.maxSize(1) - .conditionally(useSsl, c => sslHost.fold(c.withTransport.tls)(c.withTransport.tls(_))) - .newRichClient(hostPort) + .newRichClient() Await.result(Future[PostgresClientImpl] { while (!client.isAvailable) {} @@ -57,14 +63,7 @@ class IntegrationSpec extends Spec { }) } - def getBadClient = { - Postgres.Client() - .withCredentials(user, password) - .database(dbname) - .withSessionPool.maxSize(1) - .conditionally(useSsl, c => sslHost.fold(c.withTransport.tls)(c.withTransport.tls(_))) - .newRichClient("badhost:5432") - } + def getBadClient = Postgres.Client().newRichClient("badhost:5432") def cleanDb(client: PostgresClient): Unit = { val dropQuery = client.executeUpdate("DROP TABLE IF EXISTS %s".format(IntegrationSpec.pgTestTable)) @@ -362,7 +361,7 @@ class IntegrationSpec extends Spec { // this test will fail if the test DB user doesn't have permission "create an extension using CREATE EXTENSION" in { - if(user == "postgres") { + if(clientBuilder.params[Postgres.User].user == "postgres") { val client = getClient val result = client.prepareAndExecute("CREATE EXTENSION IF NOT EXISTS hstore") Await.result(result) diff --git a/src/test/scala/com/twitter/finagle/postgres/integration/NumericSpec.scala b/src/test/scala/com/twitter/finagle/postgres/integration/NumericSpec.scala index 23321a96..b9e3a7ac 100644 --- a/src/test/scala/com/twitter/finagle/postgres/integration/NumericSpec.scala +++ b/src/test/scala/com/twitter/finagle/postgres/integration/NumericSpec.scala @@ -6,29 +6,16 @@ import com.twitter.util.{Await, Future} import org.scalatestplus.scalacheck.ScalaCheckDrivenPropertyChecks class NumericSpec extends Spec with ScalaCheckDrivenPropertyChecks { - - - for { - hostPort <- sys.env.get("PG_HOST_PORT") - user <- sys.env.get("PG_USER") - password = sys.env.get("PG_PASSWORD") - dbname <- sys.env.get("PG_DBNAME") - useSsl = sys.env.getOrElse("USE_PG_SSL", "0") == "1" - } yield { - - val binaryClient = Postgres.Client() - .database(dbname) - .withCredentials(user, password) + IntegrationSpec.clientBuilder().foreach { clientBuilder => + val binaryClient = clientBuilder .withBinaryParams(true) .withBinaryResults(true) - .newRichClient(hostPort) + .newRichClient() - val textClient = Postgres.Client() - .database(dbname) - .withCredentials(user, password) + val textClient = clientBuilder .withBinaryParams(false) .withBinaryResults(false) - .newRichClient(hostPort) + .newRichClient() Await.result((textClient.query( """ diff --git a/src/test/scala/com/twitter/finagle/postgres/integration/TransactionSpec.scala b/src/test/scala/com/twitter/finagle/postgres/integration/TransactionSpec.scala index 0c828fe1..ca7302ab 100644 --- a/src/test/scala/com/twitter/finagle/postgres/integration/TransactionSpec.scala +++ b/src/test/scala/com/twitter/finagle/postgres/integration/TransactionSpec.scala @@ -5,19 +5,8 @@ import com.twitter.finagle.postgres.Spec import com.twitter.util.{Await, Future} class TransactionSpec extends Spec { - for { - hostPort <- sys.env.get("PG_HOST_PORT") - user <- sys.env.get("PG_USER") - password = sys.env.get("PG_PASSWORD") - dbname <- sys.env.get("PG_DBNAME") - useSsl = sys.env.getOrElse("USE_PG_SSL", "0") == "1" - } yield { - - val client = Postgres.Client() - .withCredentials(user, password) - .database(dbname) - .conditionally(useSsl, _.withTransport.tlsWithoutValidation) - .newRichClient(hostPort) + IntegrationSpec.clientBuilder().foreach { clientBuilder => + val client = clientBuilder.newRichClient() Await.result(client.query( """ From 6ec90266499109615c1075a718118e6c99ac1d5c Mon Sep 17 00:00:00 2001 From: Ben Spencer Date: Sat, 20 Jun 2020 17:06:41 +0100 Subject: [PATCH 2/2] Close bad test client after use. --- .../finagle/postgres/integration/IntegrationSpec.scala | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala b/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala index 3e432a41..673781df 100644 --- a/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala +++ b/src/test/scala/com/twitter/finagle/postgres/integration/IntegrationSpec.scala @@ -429,8 +429,12 @@ class IntegrationSpec extends Spec { } "client is bad" in { val badClient: PostgresClient = getBadClient - badClient.isAvailable must equal(false) - Set(Status.Busy, Status.Closed) must contain (badClient.status) + try { + badClient.isAvailable must equal(false) + Set(Status.Busy, Status.Closed) must contain (badClient.status) + } finally { + badClient.close() + } } "client is closed" in { val client: PostgresClient = getClient