LNK-4533: enhance smoke test validations phase two - #1318
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes refactor the ad-hoc reporting smoke test to adopt a data-driven validation framework. The test suite now uses configurable validation severity levels and centralized validation logic in SubmissionZipReader, replacing sequential per-step validations with consolidated resource counting and diagnostic reporting across patient files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
nvmLantana
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs (1)
100-383: Significant code duplication across patient validation methods.As noted in a past review comment,
expectedBundleCountsandexpectedEvalCountsare largely duplicative for each patient. Consider a data-driven approach where expected counts are stored in a configuration structure, reducing the six nearly-identical methods to a single parameterized method.
🧹 Nitpick comments (11)
Tests/BackendE2ETests/AdhocReportingSmokeTest.cs (2)
100-139: Inconsistent indentation in test method.Lines 111-119 are indented as if they're inside the
tryblock, but they execute before thetrystatement on line 121. While this doesn't affect runtime behavior (they execute sequentially), the inconsistent indentation reduces readability and may mislead future maintainers.🔎 Proposed fix to align indentation
{ TestConfig.AdhocReportingSmokeTestConfig.RemoveFacilityConfig = true; AdHocReportApiRequests apiE2E = new AdHocReportApiRequests(output); SubmissionZipReader submissionReportZip = new SubmissionZipReader(output); AdhocReportingSmokeTest adhocReportingSmokeTest = new AdhocReportingSmokeTest(output); MeasureLoader measureLoader = new MeasureLoader(AdminBffClient, output); Stopwatch stopwatch = new Stopwatch(); stopwatch.Start(); output.WriteLine($"Stopwatch start {DateTime.UtcNow}"); - await measureLoader.LoadAsync(); - apiE2E.Create_SingleMeasureAdHocTestFacility(); - apiE2E.Create_SingleMeasureCensusConfiguration_AdHoc(); - apiE2E.Create_SingleMeasureQueryDispatchConfig_AdHoc(); - apiE2E.Create_SingleMeasure_FHIRQueryConfigByFacility_AdHoc(); - apiE2E.Create_SingleMeasure_MontlhyQueryPlanByFacility_AdHoc(); - apiE2E.Create_SingleMeasure_DischargeQueryPlanByFacility_AdHoc(); - apiE2E.Create_SingleMeasureFHIRQueryListByFacility_AdHoc(); - apiE2E.GenerateSingleMeasureAdHocReport_ACH(); + await measureLoader.LoadAsync(); + apiE2E.Create_SingleMeasureAdHocTestFacility(); + apiE2E.Create_SingleMeasureCensusConfiguration_AdHoc(); + apiE2E.Create_SingleMeasureQueryDispatchConfig_AdHoc(); + apiE2E.Create_SingleMeasure_FHIRQueryConfigByFacility_AdHoc(); + apiE2E.Create_SingleMeasure_MontlhyQueryPlanByFacility_AdHoc(); + apiE2E.Create_SingleMeasure_DischargeQueryPlanByFacility_AdHoc(); + apiE2E.Create_SingleMeasureFHIRQueryListByFacility_AdHoc(); + apiE2E.GenerateSingleMeasureAdHocReport_ACH(); try {
103-104: Unused local variableadhocReportingSmokeTest.The variable
adhocReportingSmokeTestis declared on line 104 but never used in the method. This appears to be leftover from a previous refactor.🔎 Proposed fix
AdHocReportApiRequests apiE2E = new AdHocReportApiRequests(output); SubmissionZipReader submissionReportZip = new SubmissionZipReader(output); - AdhocReportingSmokeTest adhocReportingSmokeTest = new AdhocReportingSmokeTest(output); MeasureLoader measureLoader = new MeasureLoader(AdminBffClient, output);Tests/BackendE2ETests/TestConfig.cs (3)
23-32: Consider consolidating patient file constants.Ten individual constants (lines 23-32) are defined but only the first six are used in
SingleMeasureExpectedFiles. If constants 7-10 are only used for negative testing (validating files do NOT appear), consider documenting this distinction or grouping them separately.
53-57: Fragile patient ID extraction using hardcoded string lengths.The
Substringcalculation relies on hardcoded prefix/suffix lengths. This is brittle if the naming convention changes.🔎 Proposed fix using a more robust approach
public static readonly string[] SingleMeasureExpectedPatientIds = SingleMeasureExpectedFiles .Where(f => f.StartsWith("patient-", StringComparison.OrdinalIgnoreCase)) - .Select(f => f.Substring("patient-".Length, f.Length - "patient-".Length - ".ndjson".Length)) + .Select(f => Path.GetFileNameWithoutExtension(f).Substring("patient-".Length)) .ToArray();
118-147: Remove commented-out dead code.The
TryRunValidationandTryRunValidationAsyncmethods are entirely commented out. If they're no longer needed after the refactor to useValidationIssue, they should be deleted rather than left as commented code.Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs (6)
2-6: Mixed JSON library usage.The file uses both
Newtonsoft.Json(line 2) andSystem.Text.Json(line 4). While both work, mixing JSON libraries in the same file can lead to inconsistent behavior and maintenance overhead. Consider standardizing on one library.
18-18: HttpClient not disposed.The
HttpClientinstance is created but never disposed. WhileHttpClientis designed to be reused, in a test context without proper disposal, this could lead to socket exhaustion in long test runs.🔎 Proposed fix - implement IDisposable or use IHttpClientFactory pattern
-public class SubmissionZipReader(ITestOutputHelper output) +public class SubmissionZipReader(ITestOutputHelper output) : IDisposable { protected static readonly string api_LinkAdminBffURL = TestConfig.AdminBffBase; protected static readonly string fhirServerBaseUrl = TestConfig.InternalFhirServerBase; protected static readonly string SingleMeasureAdHocFacility = TestConfig.SingleMeasureAdHocFacility; protected static readonly string SingleMeasureAdHocAchDqmVersion = TestConfig.SingleMeasureAdHocAchDqmVersion; protected static readonly string[] SingleMeasureExpectedFiles = TestConfig.SingleMeasureExpectedFiles; protected static readonly string[] SingleMeasureExpectedPatientIDs = TestConfig.SingleMeasureExpectedPatientIds; private readonly HttpClient _client = new HttpClient(); private readonly Dictionary<string, string> _zipContents = new(); string AdHocReportGuid => TestConfig.TestContextStore.AdHocReportTrackingIdGuid; + + public void Dispose() + { + _client.Dispose(); + }
885-903: Unused methodValidateResourceTypeCounts.This static method is defined but never called anywhere in the file. If it's intended for future use, consider marking it with a TODO comment or removing it to reduce dead code.
904-983: Unused methodValidateOperationOutcomeIssuesForPatients.This method is defined but not called. If it's planned for future validation, add a TODO; otherwise, remove it.
984-1059: Unused methodValidateListSnapshotBlockForPatients.This method is defined but not called. Same recommendation as above.
1076-1083: Unused methodTruncate.The
Truncatemethod is defined but never called. TheCellmethod (line 1228) performs similar truncation logic. Consider removingTruncateif it's not needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Tests/BackendE2ETests/AdhocReportingSmokeTest.csTests/BackendE2ETests/ApiRequests/SubmissionZipReader.csTests/BackendE2ETests/TestConfig.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
⚙️ CodeRabbit configuration file
**/*.cs: TheHtmlInputSanitizerclass'sSanitize()andSanitizeAndRemove()methods should be used when dealing withstringquery parameters from REST requests.
Files:
Tests/BackendE2ETests/AdhocReportingSmokeTest.csTests/BackendE2ETests/TestConfig.csTests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
**
⚙️ CodeRabbit configuration file
**: Pull requests that have "TECH_DEBT" in the title should only contain changes related to typos, unused code, linter/IDE suggestions, swagger specification updates,
and logging improvements. These TECH_DEBT PRs must not affect core functionality. All PRs that are not considered technical debt must include information on what
testing was performed in the description of the PR. If it does not, ask the author to provide details on what testing was performed.
When reviewing code, suggest unit tests using XUnit in the following scenarios:
- If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test.
- Logic that depends on service or interface configuration — suggest tests to validate different implementations are correctly resolved.
- No network activity (HTTP calls, sockets, etc.) should appear in unit tests. Recommend using mocks (via Moq) for any external communication.
Large unit tests should be avoided; keeping unit tests small and focused on targeted business logic (i.e. string sanitization)
**: Pull requests that have DOCS in the title should only contain changes related to documentation within the /docs folder or in .md files through-out the code-base. The description
of the PR should specify what documentation was updated. Documentation updates should use EventCatalog.dev structure, where service-specific functionality should be described
in the service's index.mdx (i.e. /services/XXX/index.mdx or /domains/XXX/services/YYY/index.mdx). Configurations that are shared by multiple services should be
reflected in the /docs/docs/config files.
Files:
Tests/BackendE2ETests/AdhocReportingSmokeTest.csTests/BackendE2ETests/TestConfig.csTests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
🧠 Learnings (6)
📓 Common learnings
Learnt from: sdmcgeown
Repo: lantanagroup/link-cloud PR: 0
File: :0-0
Timestamp: 2025-07-24T17:49:23.117Z
Learning: PR #1004 in the Link Cloud project introduced comprehensive normalization operation testing functionality including a new test operation dialog, JSON validation, resource type validation, and real-time testing capabilities that require documentation updates across multiple files in the docs folder.
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1042
File: DotNet/Shared/Application/Models/PatientSubmissionModel.cs:12-12
Timestamp: 2025-08-05T18:16:51.347Z
Learning: In the Link Cloud PatientSubmissionModel, combining patient-specific and shared/common resources into a single FHIR Bundle is the intended design, as confirmed by smailliwcs. This consolidation approach is preferred over maintaining separate bundles for different resource types.
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 851
File: Tests/BackendE2ETests/AdhocReportingSmokeTest.cs:16-16
Timestamp: 2025-05-27T16:32:09.282Z
Learning: The smoke test (AdhocReportingSmokeTest) in Tests/BackendE2ETests/AdhocReportingSmokeTest.cs typically takes around 90 seconds to run locally, but uses a 5-minute timeout (MaxRetryCount = 60 with 5-second polling intervals) to account for potentially longer cloud-based runtimes.
Learnt from: amphillipsLGC
Repo: lantanagroup/link-cloud PR: 737
File: DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs:23-23
Timestamp: 2025-03-20T22:11:00.226Z
Learning: The facilityId validation in GetReportSummaries.Handle method in DotNet/Admin.BFF/Presentation/Endpoints/Aggregation/Handlers/Report/GetReportSummaries.cs will be implemented in a future phase of work by amphillipsLGC.
📚 Learning: 2025-05-27T16:32:09.282Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 851
File: Tests/BackendE2ETests/AdhocReportingSmokeTest.cs:16-16
Timestamp: 2025-05-27T16:32:09.282Z
Learning: The smoke test (AdhocReportingSmokeTest) in Tests/BackendE2ETests/AdhocReportingSmokeTest.cs typically takes around 90 seconds to run locally, but uses a 5-minute timeout (MaxRetryCount = 60 with 5-second polling intervals) to account for potentially longer cloud-based runtimes.
Applied to files:
Tests/BackendE2ETests/AdhocReportingSmokeTest.cs
📚 Learning: 2025-08-05T18:16:51.347Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1042
File: DotNet/Shared/Application/Models/PatientSubmissionModel.cs:12-12
Timestamp: 2025-08-05T18:16:51.347Z
Learning: In the Link Cloud PatientSubmissionModel, combining patient-specific and shared/common resources into a single FHIR Bundle is the intended design, as confirmed by smailliwcs. This consolidation approach is preferred over maintaining separate bundles for different resource types.
Applied to files:
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
📚 Learning: 2025-08-14T20:26:36.920Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 1062
File: DotNet/Report/Core/PatientReportSubmissionBundler.cs:206-209
Timestamp: 2025-08-14T20:26:36.920Z
Learning: In PatientReportSubmissionBundler.cs, the AddResourceToBundle method uses full URLs as unique identifiers for resources. When duplicate full URLs are encountered, it indicates identical resources, so preserving the existing resource (rather than replacing it) is the intended and correct behavior. The deduplication logic should not replace existing entries when the same full URL is found.
Applied to files:
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
📚 Learning: 2025-08-05T20:08:31.642Z
Learnt from: smailliwcs
Repo: lantanagroup/link-cloud PR: 968
File: DotNet/ServiceTests/IntegrationTests/Report/Listeners/ResourceEvaluatedListenerTests.cs:202-500
Timestamp: 2025-08-05T20:08:31.642Z
Learning: In the Link Cloud project, comprehensive content verification for .ndjson blobs and SubmitPayload event structures is intentionally covered in end-to-end testing rather than integration tests. Integration tests focus on component interactions while E2E tests verify complete submission content and blob structure.
Applied to files:
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
📚 Learning: 2025-05-06T16:07:43.281Z
Learnt from: seanmcilvenna
Repo: lantanagroup/link-cloud PR: 826
File: Tests/BackendE2ETests/TestConfig.cs:35-44
Timestamp: 2025-05-06T16:07:43.281Z
Learning: In the Link-cloud E2E testing framework, OAuth parameter validation is performed in the AuthHelper.GetBearerToken() method at runtime when a token is requested, rather than in the OAuthConfig constructor.
Applied to files:
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs
🧬 Code graph analysis (1)
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs (1)
Tests/BackendE2ETests/TestConfig.cs (4)
TestConfig(5-192)ValidationHelper(113-191)ValidationIssue(169-190)ValidationIssue(177-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Smoke Test with Docker Compose
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
Tests/BackendE2ETests/ApiRequests/SubmissionZipReader.cs (2)
81-84: Negative test for patients 7-10 is intentional.This addresses the past review comment: The constants for patients 7-10 are used here to verify they do NOT appear in the submission, which is valid negative testing for patients outside the POI list. The current implementation correctly validates that only patients in the POI list appear in the submission.
1101-1226: Well-structured validation orchestration with clear reporting.The
ValidateAllPatientsWithConfigurableSeveritymethod provides good aggregation of issues, clear console output formatting, and proper separation between warnings and errors. The approach of collecting all issues before failing (no fail-fast) aligns with the PR objectives.
smailliwcs
left a comment
smailliwcs
left a comment
There was a problem hiding this comment.
Approved (assuming updates to the resource type counts).
Signed-off-by: leolantana <167233285+leolantana@users.noreply.github.com>
a9a01be to
1c28295
Compare
3 participants
🛠️ Description of Changes
This PR significantly enhances the existing Single Measure AdHoc smoke test by adding deep, per-patient validation of submission ZIP contents, aligned with the behavior already enforced in the QA regression suite.
Patient-Scoped Validation (No Fail-Fast)
Each patient file is validated independently.
All patients are evaluated in a single test run, even if issues are found.
Results are aggregated and reported at the end, rather than failing on first error.
Output validation detects:
Missing resource types --Please make sure to have a look here. These counts are based on ResourceType counts derived from a manual evaluation of each patient input file.
ResourceType Count discrepancies --Please make sure to have a look here. These counts are based on ResourceType counts derived from a manual evaluation of each patient input file.
Missing evaluated resources
ID mismatches between bundle contents and MeasureReport.evaluatedResource
Each validation condition is assigned a severity:
Warning – informational or expected discrepancies during development
Error – data integrity violations
Severity mapping is centralized and configurable
The test output is formatted for readability:
Grouped by patient file
Grouped by validation type
Columns dynamically sized per patient
Detailed evaluatedResource breakdowns printed separately for clarity
🧪 Testing Performed
Multiple test runs, screenshot attached
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.