WOR-1817: Refactor api service tests #3033

blakery · 2024-09-16T20:24:24Z

Ticket: https://broadworkbench.atlassian.net/browse/WOR-1817

Create better testing setup for api-level tests, and apply to workspace service api.

PR checklist

Include the JIRA issue number in the PR description and title
Make sure Swagger is updated if API changes
- ...and Orchestration's Swagger too!
If you changed anything in model/, then you should publish a new official rawls-model and update rawls-model in Orchestration's dependencies.
Get two thumbsworth of PR review
Verify all tests go green, including CI tests
Squash commits and merge to develop (branches are automatically deleted after merging)
Inform other teams of any substantial changes via Slack and/or email

blakery · 2024-09-16T20:25:02Z

core/src/main/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiService.scala

@@ -57,7 +57,7 @@ trait WorkspaceApiService extends UserInfoDirectives {
          }
      } ~
        path("workspaces" / "tags") {
-          parameters('q.?, "limit".as[Int].optional) { (queryString, limit) =>
+          parameters(Symbol("q").?, "limit".as[Int].optional) { (queryString, limit) =>


This isn't functional - it's just that the previous syntax is deprecated.

blakery · 2024-09-16T20:26:13Z

core/src/main/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiService.scala

@@ -79,13 +79,6 @@ trait WorkspaceApiService extends UserInfoDirectives {
          }
        } ~
        path("workspaces" / Segment / Segment) { (workspaceNamespace, workspaceName) =>
-          /* we enforce a 6-character minimum for workspaceNamespace, as part of billing project creation.


This comment hasn't been true since the paths were re-ordered for correct path specificity.
In this PR however, we can finally demonstrate that this is true.

blakery · 2024-09-16T20:27:54Z

core/src/main/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiService.scala

                entity(as[Set[WorkspaceACLUpdate]]) { aclUpdate =>
+                  val inviteUsersNotFoundValue = inviteUsersNotFound match {


This fixes a bug discovered in testing, where if the query string contained an empty arg (eg: ?inviteUsersNotFound=), the service would throw a 500 because it tries to call .toBoolean on an empty string, which fails with an IllegalArgumentException.

blakery · 2024-09-16T20:30:08Z

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/MockApiService.scala

+  override val bucketMigrationServiceConstructor: RawlsRequestContext => BucketMigrationService = _ =>
+    mock[BucketMigrationService],
+  override val userServiceConstructor: RawlsRequestContext => UserService = _ => mock[UserService]
+)(implicit val executionContext: ExecutionContext)


The basic purpose of this is:

set up default mocks with no functionality, so you only need to specify the ones you care about

set up paths that reflect those in RawlsApiService, but without the parts that we don't care about, such as metrics and logging.

blakery · 2024-09-16T20:30:44Z

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/SubmissionApiServiceSpec.scala

@@ -1439,4 +1447,68 @@ class SubmissionApiServiceSpec extends ApiServiceSpec with TableDrivenPropertyCh
        }
    }
  }
+
+  it should "return a 201 when a comment is updated" in {


These were in the original POC - so I might as well include them.

blakery · 2024-09-16T20:32:09Z

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/WorkspaceApiServiceSpec.scala

The diff is kind of useless, because basically the entire file was replaced. I'd recommend just looking at the new file when reviewing.

…re with usage of new setup patterns

marctalbott

What does the change in coverage in the WorkspaceService look like?

marctalbott · 2024-09-17T19:25:42Z

core/src/test/scala/org/broadinstitute/dsde/rawls/webservice/MockApiService.scala

+class MockApiService(
+  val userInfo: UserInfo =
+    UserInfo(RawlsUserEmail(""), OAuth2BearerToken("token"), 123, RawlsUserSubjectId("123456789876543212349")),
+  override val openIDConnectConfiguration: OpenIDConnectConfiguration = mock[OpenIDConnectConfiguration],


Would it be worth setting these to RETURN_SMART_NULLS to help people figure out what calls aren't getting mocked correctly when they're writing a new test?

blakery · 2024-09-18T14:10:09Z

What does the change in coverage in the WorkspaceService look like?

A 3% reduction in coverage (about 40 lines).

The most significant losses are in getWorkspaceById, getBucketOptions, destroyPet, listPendingFileTransfersForWorkspace, and getBucketUsage.

I'll make a follow on ticket to add tests for those.
It will likely involve some refactoring of the service, so I think it's better to keep it separate.

blakery · 2024-09-18T14:15:39Z

Created WOR-1823 for follow-on test coverage.

blakery commented Sep 16, 2024

View reviewed changes

blakery requested review from a team, cahrens and marctalbott and removed request for a team September 16, 2024 20:32

blakery added 8 commits September 16, 2024 16:33

add mock api service and sample tests demonstrating usage

43a4398

use real routes from RawlsApiService to test full paths

fa27d51

move submissions api tests to correct test class

745773d

give MockApiService all required properties

957eb3c

temporarily add separate test class so existing setup doesn't interfe…

ab235f5

…re with usage of new setup patterns

add test for updating ACL, fix broken test case

6e37fc7

finish replacing workspace api tests

1798b23

clean up

4a46df5

blakery force-pushed the refactor-api-service-tests branch from d5172ea to 4a46df5 Compare September 16, 2024 20:33

blakery marked this pull request as ready for review September 16, 2024 20:33

marctalbott reviewed Sep 17, 2024

View reviewed changes

set default mocks to RETURNS_SMART_NULLS

9066267

marctalbott approved these changes Sep 18, 2024

View reviewed changes

aherbst-broad approved these changes Sep 18, 2024

View reviewed changes

blakery merged commit 712a4ac into develop Sep 18, 2024
29 checks passed

blakery deleted the refactor-api-service-tests branch September 18, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WOR-1817: Refactor api service tests #3033

WOR-1817: Refactor api service tests #3033

blakery commented Sep 16, 2024

blakery Sep 16, 2024

blakery Sep 16, 2024

blakery Sep 16, 2024

blakery Sep 16, 2024

blakery Sep 16, 2024

blakery Sep 16, 2024

marctalbott left a comment

marctalbott Sep 17, 2024

blakery Sep 18, 2024

blakery commented Sep 18, 2024

blakery commented Sep 18, 2024 •

edited by jira bot

Loading

		entity(as[Set[WorkspaceACLUpdate]]) { aclUpdate =>
		val inviteUsersNotFoundValue = inviteUsersNotFound match {

WOR-1817: Refactor api service tests #3033

WOR-1817: Refactor api service tests #3033

Conversation

blakery commented Sep 16, 2024

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

marctalbott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakery commented Sep 18, 2024

blakery commented Sep 18, 2024 • edited by jira bot Loading

blakery commented Sep 18, 2024 •

edited by jira bot

Loading