-
Notifications
You must be signed in to change notification settings - Fork 1
001 generator reliability hardening #21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ability hardening - Introduced a comprehensive feature specification document outlining user scenarios, requirements, success criteria, and edge cases for hardening the ActorSrcGen source generator. - Created a detailed task list organized by phases and user stories, including setup, foundation, and implementation of determinism, thread safety, and comprehensive testing. - Established clear acceptance criteria and measurable outcomes for each user story to ensure reliability and testability of the generator.
…nd enhance tests - Introduced a new diagnostics class with rules for actor definitions, including checks for step methods and entry points. - Created a coverage workflow in GitHub Actions to ensure code quality and coverage reporting. - Added helper classes for compilation and snapshot testing to streamline test setup. - Implemented integration tests for actor generation, ensuring deterministic outputs across multiple runs and varying input orders. - Developed unit tests for ActorNode and BlockNode, verifying their properties and behaviors. - Enhanced diagnostic message tests to validate expected outputs for various actor configurations. - Added generated code snapshots for various actor patterns to ensure consistency in generated outputs.
…ead safety improvements
… safety, enhance stress testing capabilities
…or and BlockGraph functionalities
- Implemented various integration tests for actor patterns, including single-step, multi-input, and ingest methods. - Added attribute validation tests to ensure correct usage of actor attributes. - Created diagnostic snapshot tests to verify proper diagnostic messages for invalid actor configurations. - Enhanced existing unit tests for cancellation handling in generator methods. - Updated generator context creation to support cancellation tokens. - Introduced new test cases for invalid ingest methods and multiple step definitions. - Verified generated code against expected outputs for various actor configurations.
…rence, and TDD workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements comprehensive reliability hardening for the ActorSrcGen source generator, transforming it from minimal test coverage (~0-20%) to enterprise-grade quality with 85%+ overall coverage and 100% critical path coverage. The PR follows a Test-Driven Development approach and addresses constitutional violations around immutability, testability, and diagnostic reporting.
Key Changes
- Test Infrastructure Expansion: Added comprehensive test suite with 50+ unit tests, 20+ integration tests, and snapshot testing infrastructure using xUnit and Verify
- Domain Model Refactoring: Introduced immutable
VisitorResultrecord to enable pure functional visitor design - Test Coverage: Achieved comprehensive coverage of Generator, ActorVisitor, ActorGenerator, and helper utilities through unit and integration tests
Reviewed changes
Copilot reviewed 93 out of 96 changed files in this pull request and generated 49 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ActorSrcGen.Tests/Usings.cs | Added global usings for System.Collections.Immutable, VerifyTests, and VerifyXunit to support new test infrastructure |
| tests/ActorSrcGen.Tests/Unit/*.cs | Comprehensive unit test suite covering TypeHelpers, RoslynExtensions, ActorVisitor, Generator, BlockNode, ActorNode, and Diagnostics with 100% critical path coverage |
| tests/ActorSrcGen.Tests/Integration/*.cs | Integration tests for snapshot patterns, stress testing, thread safety, cancellation, determinism, and diagnostic reporting |
| tests/ActorSrcGen.Tests/Snapshots/**/*.verified.cs | Snapshot test verified outputs for various actor patterns (SimpleActor, PipelineActor, MultiInputActor, IngestActor, ReceiverActor, ComplexActor, etc.) |
| tests/ActorSrcGen.Tests/Helpers/*.cs | Test helper utilities for compilation setup, snapshot verification, and test actor factory patterns |
| ActorSrcGen/Model/VisitorResult.cs | New immutable record encapsulating visitor results (actors + diagnostics) to enable pure functional design |
| ActorSrcGen/Helpers/TypeHelpers.cs | Enhanced null safety with nullable annotations, improved IsCollection to recognize ImmutableArray types, defensive null checks |
| ActorSrcGen/Templates/Actor.tt | Added null/empty handler body fallback to prevent template crashes when HandlerBody is empty |
| ActorSrcGen/Properties/AssemblyInfo.cs | Added InternalsVisibleTo for test project access to internal types |
| ActorSrcGen/IsExternalInit.cs | Shim for C# 9 record support on netstandard2.0 target |
| tests/ActorSrcGen.Tests/ActorSrcGen.Tests.csproj | Updated test project with Verify.Xunit 24.0.0, coverage configuration (85% threshold), and proper analyzer reference pattern |
| ReadMe.md | Added Testing and Diagnostics sections with coverage commands and diagnostic reference links |
| doc/DIAGNOSTICS.md | New diagnostic reference documentation for ASG0001-ASG0003 with remediation guidance |
| Contributions.md | Added TDD workflow requirements and coverage threshold expectations (85% overall, 100% critical) |
| Directory.Build.props | Updated Microsoft.SourceLink.GitHub from 1.0.111 to 1.1.1 |
| specs/001-generator-reliability-hardening/*.md | Comprehensive specification, research, data model, quick start guide, plan, tasks, and requirements checklist documentation |
| tmp-lines.ps1 | Utility script for line numbering (appears to be development tooling, not production code) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var ingest in ingesters) | ||
| { | ||
| Method = method, | ||
| NodeType = NodeType.TransformMany, | ||
| HandlerBody = $$""" | ||
| async ({{inputTypeName}} x) => { | ||
| try | ||
| { | ||
| return await {{method.Name}}(x); | ||
| } | ||
| catch(Exception e) | ||
| { | ||
| LogMessage(LogLevel.Error, $"Error in {{method.Name}}: {e.Message}\nStack Trace: {e.StackTrace}"); | ||
| return default; | ||
| } | ||
| } | ||
| """ | ||
| }; | ||
| var method = ingest.Method; | ||
| var returnsTask = string.Equals(method.ReturnType.Name, "Task", StringComparison.Ordinal) || method.ReturnType.Name.Contains("AsyncEnumerable", StringComparison.Ordinal); | ||
| if (!method.IsStatic || !returnsTask) | ||
| { | ||
| builder.Add(ActorSrcGen.Diagnostics.Diagnostics.CreateDiagnostic(ActorSrcGen.Diagnostics.Diagnostics.ASG0003, method.Locations.FirstOrDefault() ?? Location.None, method.Name)); | ||
| } | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
| Console.WriteLine("Called Synchronously"); | ||
|
|
||
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | ||
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposable 'CancellationTokenSource' is created but not disposed.
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | |
| using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); |
| var result = visitor.VisitActor(syntaxAndSymbol); | ||
| var actor = Assert.Single(result.Actors); | ||
|
|
||
| var diagnostics = new List<Diagnostic>(); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of this container are never accessed.
| } | ||
|
|
||
| return t.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat); | ||
| return t?.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat) ?? string.Empty; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition is always not null because of ... is ....
| return t?.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat) ?? string.Empty; | |
| return t.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat); |
| var src = BuildSingleStep("SingleStepActor1", "input"); | ||
| var output = Generate(src); | ||
|
|
||
| Assert.True(output.ContainsKey("SingleStepActor1.generated.cs")); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inefficient use of 'ContainsKey' and indexer.
|
|
||
| TransformBlock<string,string> _Process; | ||
|
|
||
| TransformBlock<string,string> _Start; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_Start' can be 'readonly'.
| _Start.LinkTo(_Process, new DataflowLinkOptions { PropagateCompletion = true }); | ||
| } | ||
|
|
||
| TransformBlock<string,string> _Finish; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_Finish' can be 'readonly'.
| _Start.LinkTo(_Process, new DataflowLinkOptions { PropagateCompletion = true }); | ||
| } | ||
|
|
||
| TransformBlock<string,string> _Process; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_Process' can be 'readonly'.
| _Start.LinkTo(_Finish, new DataflowLinkOptions { PropagateCompletion = true }); | ||
| } | ||
|
|
||
| TransformBlock<string,string> _Finish; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_Finish' can be 'readonly'.
|
|
||
| TransformBlock<string,string> _Finish; | ||
|
|
||
| TransformManyBlock<string,string> _Start; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Field '_Start' can be 'readonly'.
… artifacts; modify coverage.yml for improved coverage report generation
…lts directory and format
No description provided.