Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
TIP This summary will be updated as you push new changes. Give us feedback
|
@copilot did you do test driven development, and make sure you had a failing test first, that is now green, to ensure this is the correct fix? |
Honest answer: the test and fix were written in the same pass rather than strictly test-first. To verify the test is a genuine regression test (not just green-washing), I just ran through the TDD verification retroactively:
The test correctly exercises the exact behavior change (filtering out |
|
@copilot Remove the new wait logic, and run the test, and confirm that we get the same error as the user reports in the original issue. Then, afterwards, re-add the logic, and confirm that it passes. |
Done (96eec2a). The test now directly simulates the Without the fix (removing With the fix, the rebuilder is excluded from the wait list, only the regular container remains, its simulated wait completes immediately, and the test passes. |
Code ReviewThe fix correctly identifies and excludes 1. The
|
| Concern | Severity |
|---|---|
protected virtual only needed for test access — leaky abstraction |
Medium |
Blanket IResourceWithParent exclusion may be too broad in future |
Low-Medium |
| Regression test mixes concerns and simulates the wrong layer | Low |
List<string> return type on virtual method |
Low |
The core bug fix (|| r is IResourceWithParent) is correct and the fix is real. The main thing worth reconsidering before merging is whether protected virtual is the right choice, or whether the method should stay private and the override-ability be exposed through a more targeted hook like ShouldWaitForResource(IResource).
|
@copilot check the pr review for feedback. See if there is any more reliable ways to check this. Are there specific marker interfaces or anything for identifying these better? Or is there any methods built into aspire to wait properly that we can just call? |
Researched the Aspire 13.2.1 APIs thoroughly (ab5de72, 4307c79). Here's what I found: Marker interfaces investigated:
Built-in Aspire wait methods:
Runtime check added as supplementary safety net:
|
…ng IResourceWithParent resources Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/ee4f5b19-6216-4842-be89-1a193cb86be7 Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/b74cfd11-b4ba-4686-9a3d-40fb7428b591 Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Aspire 13.2.0 introduced
ProjectRebuilderResource— an internal resource automatically added perProjectResourcefor hot-reload support. It implementsIComputeResource, causing TUnit to include it in the wait list, but it never emits a healthy/running state, resulting in a guaranteed timeout.Changes
AspireFixture.ShouldWaitForResource: Newprotected virtual boolpredicate that determines whether a resource should be included in the wait list. Default implementation waits forIComputeResourceinstances unless they also implementIResourceWithParent. This is a composable extension point — subclasses can override withbase.ShouldWaitForResource(resource) && !MyCustomCheck(resource)to add custom exclusion logic.GetWaitableResourceNames: Nowprivate, usesShouldWaitForResourceinternally. No longer exposed as aprotected virtualmethod — the predicate is the intended extension point.Runtime
IsHiddensafety net: A secondary runtime check inWaitForResourcesWithFailFastAsyncusingResourceNotificationService.TryGetCurrentStateskips resources whose snapshot is markedIsHidden = true. This catches hidden internal resources (likeProjectRebuilderResource) even if they slip past the model-level filter, providing two layers of defense. The runtime filter creates a filtered copy (visibleNames) rather than mutating the input list.Regression tests: Three focused unit tests verify the
ShouldWaitForResourcepredicate directly:ShouldWaitForResource_IncludesComputeResource— regular containers are includedShouldWaitForResource_ExcludesIResourceWithParent— rebuilder-like resources are excludedShouldWaitForResource_ExcludesNonComputeResource— parameters/connection strings are excludedAspire API research
Alternatives were investigated and documented in XML docs:
IResourceWithoutLifetime— has zero implementations in Aspire 13.2.x, so it's unusable for filtering.ExplicitStartupAnnotation— public, but could over-exclude user resources that are manually started then expected to become healthy.WaitBehavior.StopOnResourceUnavailable— doesn't help; the rebuilder stays inNotStarted(not a terminal state), soWaitForResourceHealthyAsyncstill hangs.ResourceNotificationService.WaitForDependenciesAsync— different use case (waits for annotated deps of a specific resource, not "all resources").