Skip to content

LNK-5160: Improve Our Test Suite Efficiency - #1588

Merged
nvmLantana merged 1 commit into
devfrom
nvm/LNK-5160_ImproveTestSuiteEfficiency
Apr 22, 2026
Merged

LNK-5160: Improve Our Test Suite Efficiency#1588
nvmLantana merged 1 commit into
devfrom
nvm/LNK-5160_ImproveTestSuiteEfficiency

Conversation

@nvmLantana

@nvmLantana nvmLantana commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

🛠️ Description of Changes

Improve Our Test Suite Efficiency

🧪 Testing Performed

Please describe the testing that was performed on the changes included in this PR.

🧑‍🔬 Unit Testing

  • I have written or updated unit tests to cover my changes
  • Coverage: 0.0%

📓 Documentation Updated

Please update any relevant sections in the project documentation that were impacted by the changes in the PR.

Summary by CodeRabbit

  • Tests
    • Enhanced test infrastructure to support SQL Server Docker container deployment and external database connections
    • Reorganized integration tests to unit tests with expanded coverage for DataAcquisition controllers and managers
    • Added comprehensive validation and error-handling test scenarios

@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR migrates integration tests to unit tests using mocks, removes test suites from DataAcquisition controllers and managers, adds equivalent unit test coverage, updates the integration test fixture to support external SQL Server connections, and updates the CI workflow to pull SQL Server Docker images.

Changes

Cohort / File(s) Summary
CI/CD Workflow
.github/workflows/tests.yaml
Added SQL Server Docker image pull step (mcr.microsoft.com/mssql/server:2022-CU13-ubuntu-22.04) before integration tests; removed BOM marker from workflow name.
Integration Test Infrastructure
DotNet/ServiceTests/IntegrationTests/DataAcquisition/DataAcquisitionIntegrationTestFixture.cs, DotNet/ServiceTests/ServiceTests.csproj
Updated fixture to support external SQL Server via environment variable LINK_TESTS_SQL_CONNECTION_STRING; added optional container management with GUID-based test database creation and cleanup; added project folder reference for controllers integration tests.
Removed Integration Tests - Controllers
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/LogControllerTests.cs, DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs, DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryListControllerTests.cs, DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs
Deleted entire integration test classes exercising controller endpoints (Search, GetLogEntryById, UpdateLogEntry, DeleteLogEntry, Process, CancelBulk, CancelByFilter, ProcessByFilter, GetFhirConfiguration, CreateFhirConfiguration, UpdateFhirConfiguration, DeleteFhirConfiguration, etc.); these tests seeded databases and used real DbContext instances.
Removed Integration Tests - Managers
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirListQueryConfigurationManagerTests.cs, DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs, DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs
Removed integration test cases verifying exception handling and input validation; removed database drop/recreate calls and replaced hard-coded identifiers with dynamically generated ones in remaining tests.
New Unit Tests - Controllers
DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/LogControllerTests.cs, DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs
Added new unit test classes with mocked dependencies covering controller endpoints across success paths, validation failures, not-found scenarios, and exception handling.
Enhanced Unit Tests - Controllers
DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs, DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs
Added negative-path test cases for exception scenarios (EntityAlreadyExistsException, NotFoundException, ArgumentNullException, IncorrectQueryPlanOrderException) with assertions on HTTP response codes and problem details.
New Unit Tests - Managers
DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs, DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirListQueryConfigurationManagerTests.cs, DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs
Added new unit test classes with mocked repositories and services validating input validation, exception handling, and domain rule enforcement without database dependencies.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~35 minutes

Possibly related PRs

Suggested labels

config-change, test-migration, unit-tests

Suggested reviewers

  • dvargaslantana
  • seanmcilvenna
  • amphillipsLGC

Poem

🐰 Integration dreams have now taken flight,
With mocks and stubs in unit test light—
The database dances to Docker's tune,
And SQL Server joins the festive room,
Where fixtures flex with external grace! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description uses a template but lacks substantive content. The 'Description of Changes' section only repeats the title without detail. Testing and documentation sections contain only placeholder text with no concrete information provided. Provide detailed explanation of test migrations, summarize integration tests converted to unit tests, explain fixture changes and external SQL Server support, and document any testing performed.
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly reflects the main objective: migrating integration tests to unit tests with mocked dependencies while updating test infrastructure and removing obsolete integration tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nvm/LNK-5160_ImproveTestSuiteEfficiency

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (8)
DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs (1)

125-135: Consider adding a symmetric UpdateAsync_MissingFacilityId_ThrowsArgumentNull test.

CreateAsync has coverage for both missing FacilityId and missing FhirServerBaseUrl, but UpdateAsync only covers the missing URL case. Since UpdateAsync validates FacilityId first (per the manager implementation), a parallel test would close the coverage gap and keep the suite symmetric.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs`
around lines 125 - 135, Add a symmetric unit test named
UpdateAsync_MissingFacilityId_ThrowsArgumentNull that constructs the same test
pattern as UpdateAsync_MissingFhirServerBaseUrl_ThrowsArgumentNull: use
CreateManager() to get the manager, create an UpdateFhirQueryConfigurationModel
with FhirServerBaseUrl set (but omit FacilityId), then assert that calling
manager.UpdateAsync(model) throws an ArgumentNullException; this mirrors the
existing CreateAsync coverage and verifies UpdateAsync validates FacilityId
first.
DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs (1)

204-217: Unused mock setup.

DeleteFhirConfiguration does not call IFhirQueryListConfigurationQueries.GetByFacilityIdAsync (see context snippet 4); it only invokes _fhirQueryListConfigurationManager.DeleteAsync. The setup on Lines 205-207 has no effect and can be removed to keep the test focused on what the endpoint actually exercises.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs`
around lines 204 - 217, Remove the unused mock setup for
IFhirQueryListConfigurationQueries.GetByFacilityIdAsync in the test for
DeleteFhirConfiguration; the controller only calls
IFhirListQueryConfigurationManager.DeleteAsync, so keep the mock setup for
IFhirListQueryConfigurationManager.DeleteAsync and remove the
GetByFacilityIdAsync setup (referencing the test method that constructs the
controller via CreateController and calls DeleteFhirConfiguration).
DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs (1)

21-39: Minor: DataAcquisitionDbContext is never disposed.

The class holds an _dbContext field but doesn't implement IDisposable/IAsyncDisposable. xUnit creates a new test class instance per [Fact], so across the suite the InMemory provider accumulates DataAcquisitionDbContext instances (and their underlying InMemory store entries) until the test process exits. Not a correctness bug, but implementing IDisposable and disposing _dbContext keeps the runner footprint small and matches the usual pattern.

♻️ Optional refactor
-public class DataAcquisitionLogManagerTests
+public class DataAcquisitionLogManagerTests : IDisposable
 {
     ...
+    public void Dispose() => _dbContext.Dispose();
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs`
around lines 21 - 39, Add disposal for the DataAcquisitionDbContext to avoid
leaking InMemory contexts: have the DataAcquisitionLogManagerTests class
implement IDisposable (or IAsyncDisposable) and in Dispose/DisposeAsync call
_dbContext.Dispose() (or await _dbContext.DisposeAsync()), ensure you reference
the existing _dbContext field and implement the interface on the
DataAcquisitionLogManagerTests class so each test instance cleans up its
DbContext after use.
DotNet/ServiceTests/ServiceTests.csproj (1)

136-138: Empty <Folder Include> for a now-empty directory.

Per the PR summary, all controller integration tests under IntegrationTests\DataAcquisition\Controllers\ have been deleted. This <Folder Include> entry (commonly auto-added by Visual Studio to preserve an empty folder in the project) only serves to keep that empty directory visible in Solution Explorer. If the folder isn't going to be repopulated shortly, consider deleting the directory and removing this ItemGroup rather than carrying an empty placeholder.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@DotNet/ServiceTests/ServiceTests.csproj` around lines 136 - 138, The project
contains an unnecessary Folder Include for
"IntegrationTests\DataAcquisition\Controllers\" in ServiceTests.csproj that was
left after deleting all controller integration tests; remove the <ItemGroup>
entry containing the Folder Include
("IntegrationTests\DataAcquisition\Controllers\") from ServiceTests.csproj and
delete the now-empty directory from the repository (or ensure no other
references remain) so the project file no longer preserves an unused placeholder
folder.
DotNet/ServiceTests/IntegrationTests/DataAcquisition/DataAcquisitionIntegrationTestFixture.cs (1)

220-233: Consider logging swallowed cleanup failures.

The bare catch silently discards all exceptions, including unexpected ones (e.g., OperationCanceledException, auth/connection errors against the external server). For test-infra diagnostics it's helpful to at least surface the failure to the console or test output so leaking databases on a shared external server are noticed.

♻️ Suggested refinement
-                catch
-                {
-                    // Ignore cleanup failures - the next run uses a new GUID.
-                }
+                catch (Exception ex)
+                {
+                    // Best-effort cleanup; surface the failure but don't fail disposal.
+                    Console.Error.WriteLine(
+                        $"[DataAcquisitionIntegrationTestFixture] Failed to drop test database '{_testDatabaseName}': {ex.Message}");
+                }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/DataAcquisitionIntegrationTestFixture.cs`
around lines 220 - 233, The current bare catch in
DataAcquisitionIntegrationTestFixture silently swallows all exceptions during DB
cleanup; change the catch to catch (Exception ex) and write the exception
details to test output (e.g., TestContext.WriteLine or Console.Error.WriteLine)
so failures (auth/connection, cancellation, etc.) are visible for diagnostics;
keep the behavior of not rethrowing but ensure the log includes ex.Message and
ex.ToString() and reference the try block where masterConn/SqlConnection and
cmd.ExecuteNonQueryAsync are used to locate the change.
DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs (1)

815-855: GetPendingRequests_ReturnsPendingLogs leaks cross-test coupling.

This test seeds with a hardcoded FacilityId = "TestFacility" and then calls SearchAsync with no FacilityId filter, so the result set will include Pending/Failed logs seeded by every other test in the collection (they all persist in the same shared DB now that EnsureDeleted/EnsureCreated is gone). The assertions still pass because they only check "any record" and "all records have an expected status", but the test is no longer verifying the seeded rows in particular. Consider filtering by a unique FacilityId (GUID-tagged, like the other tests) and asserting on the exact pending row.

♻️ Suggested fix
-        var pendingLog = new DataAcquisitionLog
-        {
-            FacilityId = "TestFacility",
+        var facilityId = $"TestFacility_{Guid.NewGuid():N}";
+        var pendingLog = new DataAcquisitionLog
+        {
+            FacilityId = facilityId,
             Status = RequestStatus.Pending,
             ExecutionDate = DateTime.UtcNow.AddDays(-1),
             Priority = AcquisitionPriority.Normal
         };
         var completedLog = new DataAcquisitionLog
         {
-            FacilityId = "TestFacility",
+            FacilityId = facilityId,
             Status = RequestStatus.Completed,
             ExecutionDate = DateTime.UtcNow.AddDays(-1)
         };
         dbContext.DataAcquisitionLogs.AddRange(pendingLog, completedLog);
         await dbContext.SaveChangesAsync();
@@
         var result = await queries.SearchAsync(new SearchDataAcquisitionLogRequest
         {
+            FacilityId = facilityId,
             RequestStatuses = [RequestStatus.Pending, RequestStatus.Failed]
         });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs`
around lines 815 - 855, The test GetPendingRequests_ReturnsPendingLogs is
leaking cross-test data because it seeds entries with a hardcoded FacilityId and
calls queries.SearchAsync without filtering; change the seeded pendingLog and
completedLog FacilityId to a unique value (e.g., Guid.NewGuid().ToString()),
pass that same FacilityId into the SearchAsync request
(SearchDataAcquisitionLogRequest.FacilityId = uniqueId) and then assert against
the exact expected row(s) (e.g., expect a single record with Status ==
RequestStatus.Pending and matching Id/FacilityId) instead of just Any() so the
test verifies only the rows it created.
DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs (1)

216-252: Assert the intended update branch is actually exercised.

These tests configure IFhirQueryConfigurationManager.UpdateAsync(...) to throw, but they only assert the final status code. Add Verify(...) calls, or remove unused setups for validation-only cases, so the tests cannot pass through a different BadRequest/NotFound short-circuit.

♻️ Example tightening for the manager-exception path
 var objectResult = (ObjectResult)result;
 Assert.Equal((int)HttpStatusCode.NotFound, objectResult.StatusCode);
+_mocker.GetMock<IFhirQueryConfigurationManager>()
+    .Verify(x => x.UpdateAsync(It.IsAny<UpdateFhirQueryConfigurationModel>(), CancellationToken.None), Times.Once);

For UpdateFhirConfigurationNegativeTest_InvalidFacilityId, either verify UpdateAsync is never called if this is meant to test request validation, or pass a request that reaches the manager if this is meant to test ArgumentNullException handling.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs`
around lines 216 - 252, The tests UpdateFhirConfigurationNegativeTest_NotFound
and UpdateFhirConfigurationNegativeTest_InvalidFacilityId set up
IFhirQueryConfigurationManager.UpdateAsync to throw but never assert that the
controller actually invoked (or did not invoke) that manager path; update these
tests to either call Verify on the IFhirQueryConfigurationManager mock (e.g.,
Verify(x => x.UpdateAsync(It.IsAny<UpdateFhirQueryConfigurationModel>(),
CancellationToken.None), Times.Once()) for the exception-path tests) or, if the
test is intended to exercise model validation short-circuiting, change the setup
to Verify that UpdateAsync was never called (Times.Never()) and/or provide an
input that will reach the manager when testing ArgumentNullException handling;
reference the QueryConfigController.UpdateFhirConfiguration call and
IFhirQueryConfigurationManager.UpdateAsync when making the Verify assertions so
the test fails if the wrong branch is taken.
DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs (1)

313-421: Verify manager exception paths to avoid false-positive 500/400 assertions.

Several tests configure manager exceptions but only assert the final status code. Add Verify(...) calls for AddAsync/UpdateAsync, and set up any required precondition queries, so these tests prove the intended exception branch is reached.

♻️ Example tightening for the update exception tests
 _mocker.GetMock<IQueryPlanQueries>()
     .Setup(x => x.ExistsAsync(It.IsAny<string>(), It.IsAny<Frequency>(), CancellationToken.None))
     .ReturnsAsync(true);
+_mocker.GetMock<IQueryPlanQueries>()
+    .Setup(x => x.GetAsync(It.IsAny<string>(), It.IsAny<Frequency>(), CancellationToken.None))
+    .ReturnsAsync(new QueryPlanModel());
 _mocker.GetMock<IQueryPlanManager>()
     .Setup(x => x.UpdateAsync(It.IsAny<UpdateQueryPlanModel>(), CancellationToken.None))
     .ThrowsAsync(new Exception("boom"));
...
 Assert.Equal((int)HttpStatusCode.InternalServerError, objectResult.StatusCode);
+_mocker.GetMock<IQueryPlanManager>()
+    .Verify(x => x.UpdateAsync(It.IsAny<UpdateQueryPlanModel>(), CancellationToken.None), Times.Once);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs`
around lines 313 - 421, Tests that assert on controller status codes for
manager-thrown exceptions are missing verifications that the manager methods
were actually invoked and the query preconditions were set up; update the tests
(e.g., CreateQueryPlan_Exception_ReturnsInternalServerError and
UpdateQueryPlan_InvalidQueryOrder_ReturnsBadRequest /
UpdateQueryPlan_GenericException_ReturnsInternalServerError) to also Setup the
required IQueryPlanQueries.ExistsAsync responses consistently and call
_mocker.GetMock<IQueryPlanManager>().Verify(x =>
x.AddAsync(It.IsAny<CreateQueryPlanModel>(), CancellationToken.None)) or
Verify(x => x.UpdateAsync(It.IsAny<UpdateQueryPlanModel>(),
CancellationToken.None)) as appropriate to ensure the AddAsync/UpdateAsync
exception path was exercised and not skipped. Ensure the mock setups for
ExistsAsync use matching argument matchers (It.IsAny<string>(),
It.IsAny<Frequency>()) so the controller reaches the manager call before
verifying.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/LogControllerTests.cs`:
- Around line 419-432: The test for ProcessByFilter currently returns an empty
PagedConfigModel so it never exercises the branch that triggers the retrieval
workflow; update the mock setup for IDataAcquisitionLogQueries.SearchAsync to
return a PagedConfigModel containing at least one
DataAcquisitionLogSummaryModel, call controller.ProcessByFilter with the same
LogSearchParameters, and then verify that the injected
IDataAcquisitionRetrievalService (or equivalent) StartRetrievalProcessBulk
method was invoked (use the same AutoMocker/CreateController to resolve and
assert the mock call). Ensure you reference ProcessByFilter,
IDataAcquisitionLogQueries.SearchAsync, StartRetrievalProcessBulk,
CreateController and LogSearchParameters when making the change.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs`:
- Around line 230-243: The test asserts NotFound but DeleteFhirConfiguration
never returns NotFound; update the test to mock the manager's delete behavior to
match controller semantics: in the test use
mocker.GetMock<IFhirQueryListConfigurationManager>().Setup(m =>
m.DeleteAsync(It.IsAny<string>(),
It.IsAny<CancellationToken>())).ThrowsAsync(new
MissingFacilityConfigurationException(...)) and then assert the
controller.DeleteFhirConfiguration(...) returns a BadRequestObjectResult, or
alternatively mock DeleteAsync to return false and assert OkObjectResult(false);
adjust the assertion and the mock setup in the
DeleteFhirConfiguration_NonExisting_ReturnsNotFound test accordingly (use
CreateController to build the controller and reference DeleteFhirConfiguration,
IFhirQueryListConfigurationManager.DeleteAsync, and
MissingFacilityConfigurationException).

---

Nitpick comments:
In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/DataAcquisitionIntegrationTestFixture.cs`:
- Around line 220-233: The current bare catch in
DataAcquisitionIntegrationTestFixture silently swallows all exceptions during DB
cleanup; change the catch to catch (Exception ex) and write the exception
details to test output (e.g., TestContext.WriteLine or Console.Error.WriteLine)
so failures (auth/connection, cancellation, etc.) are visible for diagnostics;
keep the behavior of not rethrowing but ensure the log includes ex.Message and
ex.ToString() and reference the try block where masterConn/SqlConnection and
cmd.ExecuteNonQueryAsync are used to locate the change.

In
`@DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs`:
- Around line 815-855: The test GetPendingRequests_ReturnsPendingLogs is leaking
cross-test data because it seeds entries with a hardcoded FacilityId and calls
queries.SearchAsync without filtering; change the seeded pendingLog and
completedLog FacilityId to a unique value (e.g., Guid.NewGuid().ToString()),
pass that same FacilityId into the SearchAsync request
(SearchDataAcquisitionLogRequest.FacilityId = uniqueId) and then assert against
the exact expected row(s) (e.g., expect a single record with Status ==
RequestStatus.Pending and matching Id/FacilityId) instead of just Any() so the
test verifies only the rows it created.

In `@DotNet/ServiceTests/ServiceTests.csproj`:
- Around line 136-138: The project contains an unnecessary Folder Include for
"IntegrationTests\DataAcquisition\Controllers\" in ServiceTests.csproj that was
left after deleting all controller integration tests; remove the <ItemGroup>
entry containing the Folder Include
("IntegrationTests\DataAcquisition\Controllers\") from ServiceTests.csproj and
delete the now-empty directory from the repository (or ensure no other
references remain) so the project file no longer preserves an unused placeholder
folder.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs`:
- Around line 216-252: The tests UpdateFhirConfigurationNegativeTest_NotFound
and UpdateFhirConfigurationNegativeTest_InvalidFacilityId set up
IFhirQueryConfigurationManager.UpdateAsync to throw but never assert that the
controller actually invoked (or did not invoke) that manager path; update these
tests to either call Verify on the IFhirQueryConfigurationManager mock (e.g.,
Verify(x => x.UpdateAsync(It.IsAny<UpdateFhirQueryConfigurationModel>(),
CancellationToken.None), Times.Once()) for the exception-path tests) or, if the
test is intended to exercise model validation short-circuiting, change the setup
to Verify that UpdateAsync was never called (Times.Never()) and/or provide an
input that will reach the manager when testing ArgumentNullException handling;
reference the QueryConfigController.UpdateFhirConfiguration call and
IFhirQueryConfigurationManager.UpdateAsync when making the Verify assertions so
the test fails if the wrong branch is taken.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs`:
- Around line 204-217: Remove the unused mock setup for
IFhirQueryListConfigurationQueries.GetByFacilityIdAsync in the test for
DeleteFhirConfiguration; the controller only calls
IFhirListQueryConfigurationManager.DeleteAsync, so keep the mock setup for
IFhirListQueryConfigurationManager.DeleteAsync and remove the
GetByFacilityIdAsync setup (referencing the test method that constructs the
controller via CreateController and calls DeleteFhirConfiguration).

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs`:
- Around line 313-421: Tests that assert on controller status codes for
manager-thrown exceptions are missing verifications that the manager methods
were actually invoked and the query preconditions were set up; update the tests
(e.g., CreateQueryPlan_Exception_ReturnsInternalServerError and
UpdateQueryPlan_InvalidQueryOrder_ReturnsBadRequest /
UpdateQueryPlan_GenericException_ReturnsInternalServerError) to also Setup the
required IQueryPlanQueries.ExistsAsync responses consistently and call
_mocker.GetMock<IQueryPlanManager>().Verify(x =>
x.AddAsync(It.IsAny<CreateQueryPlanModel>(), CancellationToken.None)) or
Verify(x => x.UpdateAsync(It.IsAny<UpdateQueryPlanModel>(),
CancellationToken.None)) as appropriate to ensure the AddAsync/UpdateAsync
exception path was exercised and not skipped. Ensure the mock setups for
ExistsAsync use matching argument matchers (It.IsAny<string>(),
It.IsAny<Frequency>()) so the controller reaches the manager call before
verifying.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs`:
- Around line 21-39: Add disposal for the DataAcquisitionDbContext to avoid
leaking InMemory contexts: have the DataAcquisitionLogManagerTests class
implement IDisposable (or IAsyncDisposable) and in Dispose/DisposeAsync call
_dbContext.Dispose() (or await _dbContext.DisposeAsync()), ensure you reference
the existing _dbContext field and implement the interface on the
DataAcquisitionLogManagerTests class so each test instance cleans up its
DbContext after use.

In
`@DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs`:
- Around line 125-135: Add a symmetric unit test named
UpdateAsync_MissingFacilityId_ThrowsArgumentNull that constructs the same test
pattern as UpdateAsync_MissingFhirServerBaseUrl_ThrowsArgumentNull: use
CreateManager() to get the manager, create an UpdateFhirQueryConfigurationModel
with FhirServerBaseUrl set (but omit FacilityId), then assert that calling
manager.UpdateAsync(model) throws an ArgumentNullException; this mirrors the
existing CreateAsync coverage and verifies UpdateAsync validates FacilityId
first.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6c69dbdd-71d3-4f61-b311-aaaf9c5eda80

📥 Commits

Reviewing files that changed from the base of the PR and between 58b21e8 and 6ae1d0b.

📒 Files selected for processing (17)
  • .github/workflows/tests.yaml
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/LogControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryListControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/DataAcquisitionIntegrationTestFixture.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirListQueryConfigurationManagerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs
  • DotNet/ServiceTests/ServiceTests.csproj
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/LogControllerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryListControllerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/DataAcquisitionLogManagerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirListQueryConfigurationManagerTests.cs
  • DotNet/ServiceTests/UnitTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs
💤 Files with no reviewable changes (6)
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirListQueryConfigurationManagerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Managers/FhirQueryConfigurationManagerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryPlanConfigControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryListControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/QueryConfigControllerTests.cs
  • DotNet/ServiceTests/IntegrationTests/DataAcquisition/Controllers/LogControllerTests.cs

@nvmLantana
nvmLantana merged commit ab972ad into dev Apr 22, 2026
26 checks passed
@nvmLantana
nvmLantana deleted the nvm/LNK-5160_ImproveTestSuiteEfficiency branch April 22, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants