Repository navigation
fix(ci): bound test sessions with a per-session timeout - #547
Merged
Merged
Conversation
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…ribute Complete rewrite of ObserverGenerator.cs to extract the payload type from the [Observer(typeof(TPayload))] attribute constructor argument and generate type-safe Subscribe/Publish methods. Key features: - Extract TPayload from attribute constructor (typeof(TPayload)) - Generate Subscribe(Action<TPayload>) and Subscribe(Func<TPayload, ValueTask>) - Generate Publish(TPayload) and PublishAsync(TPayload, CancellationToken) - Implement proper snapshot semantics for thread-safe iteration - Support three threading policies: * SingleThreadedFast: No locking, just a List * Locking: Use lock() for thread safety (default) * Concurrent: Use Immutable collections for lock-free operation - Support three exception policies: * Continue: Invoke all handlers, call optional OnSubscriberError hook * Stop: Rethrow first exception * Aggregate: Collect all exceptions and throw AggregateException - Support RegistrationOrder (FIFO) and Undefined order policies - Support all target type kinds (class, struct, record class, record struct) - Handle structs without field initializers (C# 11+ compatibility) - Nested private Subscription class implementing IDisposable - Idempotent, thread-safe disposal - Clean, deterministic code generation with proper nullability annotations The generator now follows the same pattern as StrategyGenerator for extracting constructor arguments and generating clean, focused implementation code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ty issues - Add ObserverAttribute(Type payloadType) constructor to accept payload type - Add PayloadType property to ObserverAttribute - Update documentation example to show [Observer(typeof(Temperature))] - Fix critical thread safety bugs in lock usage: * Use lock field assignment to ensure same lock object is used * Change 'lock (_lock ?? new object())' to proper initialization - Add defensive null check for Attributes array access - Ensure lock object is initialized before use in Publish and Unsubscribe These fixes address code review feedback and ensure proper thread-safe operation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
- Created comprehensive documentation at docs/generators/observer.md
- Overview and motivation
- Basic usage examples with sync and async handlers
- Configuration options (threading, exceptions, ordering)
- Complete API reference (Subscribe, Publish, PublishAsync)
- Best practices and performance considerations
- Common patterns (observable properties, event aggregator)
- Diagnostics reference (PKOBS001-003)
- Troubleshooting guide
- Added real-world examples in src/PatternKit.Examples/ObserverGeneratorDemo/
- TemperatureMonitor.cs: Basic Observer usage with temperature sensors
- Demonstrates sync handlers, multiple subscribers
- Exception handling with OnSubscriberError
- Subscription lifecycle management
- NotificationSystem.cs: Advanced async patterns
- Multi-channel notifications (Email, SMS, Push)
- Async handlers with PublishAsync
- Exception policies (Continue vs Aggregate)
- Cancellation token support
- README.md: Comprehensive example documentation
- Quick start guide
- Configuration examples
- Common patterns
- Running instructions
- Updated docs/generators/toc.yml to include observer.md
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use positional parameters instead of named parameters with incorrect casing in Notification record instantiation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ion, fire-and-forget async, disable struct support Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…ixed handler test, verify struct diagnostics Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…ets.Struct, improve async error handling, remove unused using Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…y Subscription.Dispose Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
… validate enum ranges, fix ForceAsync config, assert EmitResult, document dependencies Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…n, fix ConcurrentBag race, update nested type examples, clarify hub attributes not implemented, merge main Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…nd-forget docs, add AttributeTargets.Struct for PKOBS003 Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…andling, add discard for Task.Run to avoid CS4014 Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…berError usage, add System.Linq using for ImmutableList.ToArray() Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
…ncurrentBag compaction on disposal, update docs Co-authored-by: JerrettDavis <2610199+JerrettDavis@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…o copilot/create-observer-pattern-generator # Conflicts: # src/PatternKit.Examples/ObserverGeneratorDemo/NotificationSystem.cs # src/PatternKit.Examples/ObserverGeneratorDemo/TemperatureMonitor.cs # src/PatternKit.Generators/AnalyzerReleases.Unshipped.md # src/PatternKit.Generators/Observer/ObserverGenerator.cs # test/PatternKit.Generators.Tests/ObserverGeneratorTests.cs # test/PatternKit.Generators.Tests/PatternKit.Generators.Tests.csproj # test/PatternKit.Generators.Tests/packages.lock.json
…er-pattern-generator
Recovers the CI hardening that was parked in the stash on docs/fix-toc-coverage and adapts it to the current per-project test layout. Two of the stashed hunks are already superseded upstream and are not reapplied: the 10-minute test-step budget and the 30-minute release-job budget. Main now uses 45/60-minute step budgets and 75/90-minute job budgets, which are tuned for the split per-project test runs that replaced the old single `dotnet test PatternKit.slnx` invocation. Tightening them back down would break CI. The portable piece is the per-test-session guard, which main does not have in any form. `-- RunConfiguration.TestSessionTimeout=300000` (5 minutes) caps a single test session so one hung test cannot consume an entire step budget. It is applied to all eight test invocations across the pr-checks and release jobs, matching the implementation on fix/ci-test-timeout.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
An extra -- separator in eight CI invocations causes VSTest to reject the commands before running tests, blocking both pr-checks and release.
1 open issue
What changed in this PR
This PR aims to limit hung test sessions to five minutes within the existing per-project CI test layout.
Changes:
- Adds
RunConfiguration.TestSessionTimeout=300000to all eightdotnet testinvocations acrosspr-checksandrelease. - Preserves existing step/job timeouts and coverage settings.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds per-session timeout arguments to four test-project commands in each job. |
Review effort: Balanced
| --logger "trx;LogFilePrefix=core-test-results" \ | ||
| -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.Format=cobertura | ||
| -- DataCollectionRunSettings.DataCollectors.DataCollector.Configuration.Format=cobertura \ | ||
| -- RunConfiguration.TestSessionTimeout=300000 |
Contributor
Code Coverage |
Contributor
Test Results 12 files 12 suites 12m 35s ⏱️ Results for commit 1bd22dd. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #547 +/- ##
=========================================
Coverage 97.44% 97.44%
=========================================
Files 611 611
Lines 49926 49926
Branches 3114 34 -3080
=========================================
+ Hits 48650 48651 +1
+ Misses 1276 1275 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
🔍 PR Validation ResultsVersion: `` ✅ Validation Steps
📊 ArtifactsDry-run artifacts have been uploaded and will be available for 7 days. This comment was automatically generated by the PR validation workflow. |
This was referenced Nov 9, 2026
Merged
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.
Recovers the CI hardening that was parked in the stash on
docs/fix-toc-coverageand adapts it to the current per-project test layout.What changed
Adds
-- RunConfiguration.TestSessionTimeout=300000(5 minutes) to all eightdotnet testinvocations across thepr-checksandreleasejobs.Why
A single hung test could previously consume an entire test step's budget. This caps one test session so the step timeout becomes a reliable backstop rather than the first line of defence. Main has no per-session guard in any form today.
What was deliberately not applied
Two hunks from the original stash are already superseded upstream and are not reapplied:
timeout-minutes: 10(test step)45dotnet test PatternKit.slnxinvocation. Main now runs four test projects separately, so tightening back to 10 minutes would break CI.timeout-minutes: 30(release job)90The stash also carried
Exclude="[*Tests]*"coverage filters. Those are not portable either — main now invokes each test project directly, so that filter would exclude the very assemblies being measured.Provenance
The stashed change was written against the older single-
dotnet testlayout and mirrored the implementation onfix/ci-test-timeout. Only the portable per-session guard is carried forward; the superseded timeout values are left in the stash entry, which is retained.🤖 Generated with Claude Code