test(report): pin the property and worker lookup fixes from #1078 and #1079 - #1109
Open
renemadsen wants to merge 1 commit into
Open
test(report): pin the property and worker lookup fixes from #1078 and #1079#1109renemadsen wants to merge 1 commit into
renemadsen wants to merge 1 commit into
Conversation
…1079 BackendConfigurationReportService had no test coverage at all, and it has now produced two production bugs of the same shape: an unguarded .First() or null dereference inside the per-planning-case loop, which aborts the entire report because both GenerateReport and GenerateReportV2 wrap their whole body in a single try/catch. Add an integration fixture covering both fixes: - #1078: a planning case whose property cannot be resolved must still be reported with a blank PropertyName. Covered for both the live AreaRulePlannings row and the AreaRulesPlanningVersions fallback, in both methods - all four call sites the fix touched. - #1079: in GenerateReportV2, a missing SDK case must be skipped rather than throwing, a site with no active SiteWorkers row must yield a blank EmployeeNo, and a soft-deleted site worker must not win over the active one. Two positive controls (one per method) assert that a resolvable property still yields its real name, so a future "blank everything" over-fix fails. Every test asserts on the produced items, never on Success alone: GenerateReport unconditionally appends a trailing model and so reports success even when zero rows were built, which would make a Success-only assertion pass vacuously whenever the seed is wrong. Validated by reverse-applying each fix and confirming the matching tests fail - including a targeted revert of only the live-branch call sites, to prove the new tests pin code the fallback tests do not reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A9DDHNWkLapbuXHjFSqFt9
There was a problem hiding this comment.
Pull request overview
Adds new MariaDB-backed integration regression tests to pin two previously production-breaking failure modes in BackendConfigurationReportService report generation (property resolution and worker lookup), ensuring per-case failures degrade to blank cells / skipped rows rather than aborting the entire report.
Changes:
- Introduces an integration test fixture covering
GenerateReportandGenerateReportV2behavior when property resolution fails (liveAreaRulePlanningsand version fallback branches). - Adds
GenerateReportV2regression coverage for missing SDK cases and for worker resolution (no site worker, removed site worker).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+212
to
+229
| private async Task<Worker> SeedWorker(string employeeNo) | ||
| { | ||
| var worker = new Worker | ||
| { | ||
| FirstName = "Regression", | ||
| LastName = employeeNo, | ||
| Email = $"{Guid.NewGuid():N}@example.com", | ||
| EmployeeNo = employeeNo, | ||
| MicrotingUid = Random.Shared.Next(100000, 999999), | ||
| WorkflowState = Constants.WorkflowStates.Created, | ||
| CreatedAt = DateTime.UtcNow, | ||
| UpdatedAt = DateTime.UtcNow, | ||
| Version = 1 | ||
| }; | ||
| await MicrotingDbContext!.Workers.AddAsync(worker); | ||
| await MicrotingDbContext.SaveChangesAsync(); | ||
| return worker; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression coverage for the two report-service fixes, #1078 and #1079.
BackendConfigurationReportServicehad no test coverage at all — zero references in either test project — and it has now produced two production bugs of the same shape: an unguarded.First(...)or null dereference inside the per-planning-case loop. Because bothGenerateReportandGenerateReportV2wrap their entire body in a singletry/catch, each of those is a report-wide outage rather than one bad cell.Tests (8)
GenerateReport_WhenPropertyIsUnresolvable_...AreaRulesPlanningVersionsfallbackGenerateReport_WhenLivePlanningPropertyIsUnresolvable_...AreaRulePlanningsrowGenerateReportV2_WhenPropertyIsUnresolvable_...GenerateReportV2_WhenLivePlanningPropertyIsUnresolvable_...GenerateReport_WhenPropertyResolves_ReturnsItemWithPropertyNameGenerateReportV2_WhenSiteHasNoWorker_ReturnsItemWithBlankEmployeeNoGenerateReportV2_WhenCaseIsMissing_SkipsCaseAndReturnsOtherItemsdbCaseguard ordering — provescontinue, not abortGenerateReportV2_WhenSiteWorkerIsRemoved_UsesActiveSiteWorkerWorkflowState != "removed"filterBetween them these cover all four
Properties.First(...)call sites that #1078 patched — the liveAreaRulePlanningsbranch and the version fallback, in each method.Why these aren't vacuous
GenerateReportunconditionally appends a trailingOldReportEformModeland returnsSuccess = true, so aSuccess-only assertion passes even when zero real rows were built. Every test therefore asserts item count, the seeded row's id, and the actual field value.ReportEformItemModel.PropertyName/.EmployeeNodefault tonull(no initializer), soIs.EqualTo("")genuinely pins the?? ""assignment rather than passing by model default.Validation
Each fix was reverse-applied and the suite re-run:
Sequence contains no elements/ wrapped LINQ-parameter NRE; only the positive control passed, by designpropertyName = ""That third row is the point: it proves the new tests pin code the pre-existing ones never reach.
Reviewed by subagent; both Important findings (the unpinned live branch, and the missing V2 positive control) were fixed before this PR. Two doc-comment accuracy nits also fixed — the fixture previously implied the removed-site-worker test pinned
OrderBy(x => x.Id), which by id arithmetic it does not; it pins theWorkflowStatefilter only.Notes
BackendConfigurationReportService.csis byte-identical tostable.HashSetfrom fix(report): guard missing case, site worker and worker in GenerateReportV2 #1079. AssertingILoggercall counts through NSubstitute is brittle generics work, and a regression there is log noise, not incorrect output.🤖 Generated with Claude Code
https://claude.ai/code/session_01A9DDHNWkLapbuXHjFSqFt9