Conversation
…ructors Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/6228d812-f169-447b-9137-5d2a605f3bb3 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1780 +/- ##
==========================================
+ Coverage 58.73% 61.20% +2.46%
==========================================
Files 275 285 +10
Lines 8143 8405 +262
Branches 946 974 +28
==========================================
+ Hits 4783 5144 +361
+ Misses 3134 3026 -108
- Partials 226 235 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WhitWaldo
left a comment
There was a problem hiding this comment.
Could you add another unit test or two that also validates that the analyzers works for primary constructors?
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/b27c2122-2b8c-40de-9e9b-994937c32ea6 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Done in 98e9ae8. The analyzer now also checks primary constructor parameter lists (
|
|
@copilot Is your implementation broad enough to handle inheritance as well? Do you anticipate any other scenarios in which this PR might be improved to cover? |
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/f9d7f462-5828-4ea5-aac1-1866aa31f83b Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Yes, inheritance is fully handled — The full coverage matrix is now:
One scenario intentionally out of scope: the abstract intermediate class itself (e.g. |
Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/ff6d6499-b783-45d5-95b5-e5a4f2386331 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
|
@copilot Still seeing open test failures |
The analyzer/code-fix unit tests all pass — 54/54 across net8, net9, and net10. The CI failure is in the |
|
@copilot I'm still seeing test failures. Please proceed. |
…prevent connection-refused failures in GrpcProxy integration tests Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/1c88f9b4-7974-4f68-aec1-d06b5fb32b85 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
…fic SocketException catch Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/1c88f9b4-7974-4f68-aec1-d06b5fb32b85 Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
The root cause was in Fixed in commits
All 54 analyzer unit tests still pass, and the GrpcProxy integration tests should no longer hit connection-refused errors. |
WhitWaldo
left a comment
There was a problem hiding this comment.
Needs a tweak to ensure it's limited in application to Dapr Workflows
…n-Dapr generic class test Agent-Logs-Url: https://github.com/dapr/dotnet-sdk/sessions/4e747a81-49f9-4efc-a86b-01325971198e Co-authored-by: WhitWaldo <2238529+WhitWaldo@users.noreply.github.com>
Done in |
Description
Adds a new Roslyn analyzer (DAPR1305) that warns when a
Workflow<,>subclass (direct or indirect) ofDapr.Workflow.Abstractions.Workflow<,>declares constructor parameters, since dependency injection via constructors is not supported in the Dapr workflow runtime. Also provides a code fix that removes the offending parameter(s) from both regular and primary constructors.Also fixes a pre-existing race condition in the
Dapr.E2E.TestgRPC proxy integration tests where tests would run before the gRPC app port was accepting connections, causing transientUnavailable/ connection-refused failures.Changes
WorkflowDependencyInjectionAnalyzer): fires on any constructor parameter in a direct or indirectWorkflow<,>subclass, covering both regular constructors and C# 12 primary constructors; restricted to types deriving fromWorkflow<,>in theDapr.Workflow.Abstractionsassembly via an assembly-identity check inGetWorkflowBaseType()WorkflowDependencyInjectionCodeFixProvider): removes the flagged parameter from the constructor's parameter list; correctly strips leading whitespace trivia only when the first parameter is removed, leaving remaining parameters clean for both regular and primary constructor formsCompilationExtensions.GetWorkflowBaseType()(updated): now verifiesContainingAssembly.Name == "Dapr.Workflow.Abstractions"after the metadata-name lookup, preventing false positives on any user-defined type that happens to share the fully-qualified nameDapr.Workflow.Workflow<,>VerifyCodeFixtest helper (updated): filters collected diagnostics to analyzer-owned IDs (eliminates incidental CS5001/CS9113 noise), relaxesAssert.Single→Assert.NotEmptyto support multi-diagnostic scenarios, and adds adiagnosticIndexoverload so individual parameters in a multi-param list can be targeted by testsWorkflowActivity(no diagnostic), and a non-Dapr generic base class (no diagnostic)DaprTestApp(updated): exposesAppPortproperty (set whenUseAppPort = true) so callers can observe the allocated app portDaprTestAppLifecycle.InitializeAsync()(updated): after the Dapr sidecar health check passes, if the app uses gRPC protocol with a known app port, callsWaitForAppPortAsync()which retries a TCP connect to the port every 250 ms (up to 30 s) before returning — eliminating the connection-refused race inGrpcProxyTestsIssue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: