PR Feedback

broadinstitute · Apr 27, 2021 · 64e80f6 · 64e80f6
1 parent 4e4d78c
commit 64e80f6
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 35 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -3,7 +3,7 @@ jdk:
 dist: trusty
 language: scala
 scala:
-- 2.12.8
+- 2.13.5
 sudo: required
 services:
 - docker

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala
@@ -14,13 +14,11 @@ import org.broadinstitute.dsde.workbench.sam.config.{LiquibaseConfig, SwaggerCon
 import org.broadinstitute.dsde.workbench.sam.dataAccess.{AccessPolicyDAO, DirectoryDAO, MockAccessPolicyDAO, MockDirectoryDAO, MockRegistrationDAO}
 import org.broadinstitute.dsde.workbench.sam.model.{ResourceActionPattern, ResourceRole, ResourceRoleName, ResourceType, ResourceTypeName, SamResourceActions}
 import org.broadinstitute.dsde.workbench.sam.service._
-import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import org.scalatest.concurrent.ScalaFutures
 
 import scala.collection.concurrent.TrieMap
 import scala.collection.mutable
 import scala.concurrent.ExecutionContext
-import scala.concurrent.duration.DurationInt
 
 /**
   * Created by dvoet on 7/14/17.
@@ -74,17 +72,7 @@ object TestSamRoutes {
     val resourceTypesWithAdmin = resourceTypes + (resourceTypeAdmin.name -> resourceTypeAdmin)
     // need to make sure MockDirectoryDAO and MockAccessPolicyDAO share the same groups
     val groups: mutable.Map[WorkbenchGroupIdentity, WorkbenchGroup] = policies.getOrElse(new TrieMap())
-    val directoryDAO = new MockDirectoryDAO(groups) {
-      override def checkStatus(samRequestContext: SamRequestContext): Boolean = {
-        dbRef.inLocalTransaction { session =>
-          if (session.connection.isValid((2 seconds).toSeconds.intValue())) {
-            true
-          } else {
-            false
-          }
-        }
-      }
-    }
+    val directoryDAO = new MockDirectoryDAO(groups)
     val registrationDAO = new MockRegistrationDAO()
     val googleDirectoryDAO = new MockGoogleDirectoryDAO()
     val policyDAO = policyAccessDAO.getOrElse(new MockAccessPolicyDAO(Map.empty[ResourceTypeName, ResourceType], groups))

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/StatusServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/StatusServiceSpec.scala
@@ -37,22 +37,6 @@ class StatusServiceSpec extends AnyFreeSpec with Matchers with BeforeAndAfterAll
     system.terminate()
   }
 
-  // We need to mock this because Postgres does some automatic setup and gets unhappy with multiple instances of its DAO existing at once.
-  private def mockDirectoryDAOWithPostgresStatusCheck(dbReferenceOverride: DbReference = dbReference) = {
-    val mockDirectoryDAO = new MockDirectoryDAO {
-      override def checkStatus(samRequestContext: SamRequestContext): Boolean = {
-        dbReferenceOverride.inLocalTransaction { session =>
-          if (session.connection.isValid((2 seconds).toSeconds.intValue())) {
-            true
-          } else {
-            false
-          }
-        }
-      }
-    }
-    mockDirectoryDAO
-  }
-
   private def createLdapDaoWithConnectionPool(connectionPoolOverride: LDAPConnectionPool = connectionPool) = {
     new LdapRegistrationDAO(connectionPoolOverride, directoryConfig, TestSupport.blockingEc)
   }
@@ -63,16 +47,18 @@ class StatusServiceSpec extends AnyFreeSpec with Matchers with BeforeAndAfterAll
     }, dbReference, pollInterval = 10 milliseconds)
   }
 
-  private def directoryDAOWithAllUsersGroup(dbReferenceOverride: DbReference = dbReference) = {
-    val directoryDAO = mockDirectoryDAOWithPostgresStatusCheck(dbReferenceOverride)
+  private def directoryDAOWithAllUsersGroup(response: Boolean = true) = {
+    val directoryDAO = new MockDirectoryDAO {
+      override def checkStatus(samRequestContext: SamRequestContext): Boolean = response
+    }
     directoryDAO.createGroup(BasicWorkbenchGroup(NoExtensions.allUsersGroupName, Set.empty, allUsersEmail), samRequestContext = samRequestContext).unsafeRunSync()
     directoryDAO
   }
 
   private def noOpenDJGroups = {
     val connectionPoolOverride = new LDAPConnectionPool(new LDAPConnection(dirURI.getHost, dirURI.getPort, directoryConfig.user, directoryConfig.password), directoryConfig.connectionPoolSize)
     connectionPoolOverride.close()
-    newStatusService(mockDirectoryDAOWithPostgresStatusCheck(), createLdapDaoWithConnectionPool(connectionPoolOverride))
+    newStatusService(new MockDirectoryDAO, createLdapDaoWithConnectionPool(connectionPoolOverride))
   }
 
   private def ok = newStatusService(directoryDAOWithAllUsersGroup(), createLdapDaoWithConnectionPool(connectionPool))
@@ -87,7 +73,7 @@ class StatusServiceSpec extends AnyFreeSpec with Matchers with BeforeAndAfterAll
   private def failingDatabase = {
     // background database configured to connect to non existent database
     val dbReferenceOverride = DbReference(DatabaseNames.Background, TestSupport.blockingEc)
-    val service = new StatusService(directoryDAOWithAllUsersGroup(dbReferenceOverride), createLdapDaoWithConnectionPool(), NoExtensions, dbReferenceOverride, pollInterval = 10 milliseconds)
+    val service = new StatusService(directoryDAOWithAllUsersGroup(false), createLdapDaoWithConnectionPool(), NoExtensions, dbReferenceOverride, pollInterval = 10 milliseconds)
     service
   }
 
@@ -96,7 +82,7 @@ class StatusServiceSpec extends AnyFreeSpec with Matchers with BeforeAndAfterAll
       ("ok", ok, StatusCheckResponse(true, Map(OpenDJ -> SubsystemStatus(true, None), GoogleGroups -> SubsystemStatus(true, None), Database -> SubsystemStatus(true, None)))),
       ("noOpenDJGroups", noOpenDJGroups, StatusCheckResponse(false, Map(OpenDJ -> SubsystemStatus(false, Option(List(s"LDAP database connection invalid or timed out checking"))), GoogleGroups -> SubsystemStatus(true, None), Database -> SubsystemStatus(true, None)))),
       ("failingExtension", failingExtension, StatusCheckResponse(false, Map(OpenDJ -> SubsystemStatus(true, None), GoogleGroups -> SubsystemStatus(false, Option(List(s"bad google"))), Database -> SubsystemStatus(true, None)))),
-      ("failingDatabase", failingDatabase, StatusCheckResponse(false, Map(OpenDJ -> SubsystemStatus(true, None), Database -> SubsystemStatus(false, Option(List("The connection attempt failed."))))))
+      ("failingDatabase", failingDatabase, StatusCheckResponse(false, Map(OpenDJ -> SubsystemStatus(true, None), Database -> SubsystemStatus(false, Option(List("Postgres database connection invalid or timed out checking"))))))
     )
   }