chore(testing): ConfigureAwait sweep + EnvConfig surface tightening - #89
Conversation
Three thematic cleanups in pre-existing WIP:
1. ConfigureAwait(false) on every await across the Testing/WebTesting
library (LogAssert, GeneratorTestEngine, SolutionRefactoringTest,
Test, MSBuild builders/fixtures, Kestrel/Integration test bases).
Standard library guidance for code that ships to consumers.
2. Examples cleanup:
- MissingCancellationTokenAnalyzer: nest the property pattern
(Property: { Name, ContainingType }) instead of two parallel
Property.* paths.
- MissingCancellationTokenFixer: collapse three sequential guard
ifs into one short-circuit chain.
3. EnvConfig: collapse min/max boundary checks into the parse guard,
and narrow ReadString from public to private — it's an internal
helper for the typed Read* methods; external callers should use
Environment.GetEnvironmentVariable directly.
Breaking change: EnvConfig.ReadString is no longer public.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary by CodeRabbit
WalkthroughThis PR systematically adds ChangesCancellation Token Analysis & Code Fixing
Async Synchronization Context Standardization
Environment Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of mechanical cleanups across the repo: standardizing library-style async usage in the Testing/WebTesting surfaces, tightening the EnvConfig API surface in the runtime utilities, and simplifying a couple of Examples analyzer/code-fix patterns.
Changes:
- Sweeps
ConfigureAwait(false)across async code paths inANcpLua.Roslyn.Utilities.Testing(MSBuild helpers, generator/refactoring harnesses, web test bases, log assertions). - Tightens
EnvConfigby makingReadStringa private helper and consolidatingReadIntbounds validation into the parse guard. - Simplifies patterns in the example xUnit cancellation analyzer and code fix (nested property pattern; consolidated guard clauses).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ANcpLua.Roslyn.Utilities/Text/EnvConfig.cs | Narrows ReadString to a private helper and streamlines ReadInt validation. |
| src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/KestrelTestBase.cs | Adds ConfigureAwait(false) to async teardown disposal. |
| src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/IntegrationTestBase.cs | Adds ConfigureAwait(false) to async teardown disposal. |
| src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/KestrelTestBase.cs | Adds ConfigureAwait(false) to async teardown disposal. |
| src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/IntegrationTestBase.cs | Adds ConfigureAwait(false) to async one-time teardown disposal. |
| src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/KestrelTestBase.cs | Adds ConfigureAwait(false) to IAsyncLifetime disposal path. |
| src/ANcpLua.Roslyn.Utilities.Testing/Test.cs | Adds ConfigureAwait(false) to generator test execution. |
| src/ANcpLua.Roslyn.Utilities.Testing/SolutionRefactoringTest.cs | Adds ConfigureAwait(false) across async refactoring test workflow. |
| src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/ProjectBuilder.cs | Adds ConfigureAwait(false) to MSBuild execution, file reads, and SARIF/binlog loading. |
| src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageTestBase.cs | Adds ConfigureAwait(false) to fluent build awaits. |
| src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageProjectBuilder.cs | Adds ConfigureAwait(false) to SDK resolution, process execution, retry delay, and artifact reads. |
| src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/NuGetPackageFixture.cs | Adds ConfigureAwait(false) to cache prewarm, disposal, file writes, and restore execution. |
| src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/DotNetSdkHelpers.cs | Adds ConfigureAwait(false) to lock acquisition, HTTP/download/extract, and TAR iteration/copy. |
| src/ANcpLua.Roslyn.Utilities.Testing/LogAssert.cs | Adds ConfigureAwait(false) to async polling helpers. |
| src/ANcpLua.Roslyn.Utilities.Testing/GeneratorTestEngine.cs | Adds ConfigureAwait(false) to compilation creation and run-twice execution. |
| src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationCodeFixes/MissingCancellationTokenFixer.cs | Collapses sequential guard clauses into a single short-circuit check. |
| src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationAnalyzer/MissingCancellationTokenAnalyzer.cs | Uses a nested property pattern for IPropertyReferenceOperation matching. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87eb7f97f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| /// <summary>Trimmed string value of <paramref name="name" />, or <paramref name="defaultValue" /> when unset/whitespace.</summary> | ||
| public static string? ReadString(string name, string? defaultValue = null) | ||
| private static string? ReadString(string name, string? defaultValue = null) |
There was a problem hiding this comment.
Changing EnvConfig.ReadString from public to private removes an existing public member from a shipped utility type, so any downstream code that currently calls this helper will fail to compile after upgrading. This is a source-breaking API change introduced in a cleanup commit; unless a breaking-version bump is planned, keep the method public (or deprecate it first) to preserve compatibility.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationCodeFixes/MissingCancellationTokenFixer.cs`:
- Around line 51-55: The current guard in MissingCancellationTokenFixer prevents
the code fix from running if DiagnosticPropertyNames.ParameterName is absent;
change the logic so that ApplyAsync only treats
DiagnosticPropertyNames.ParameterIndex as mandatory (keep the TryGetValue +
int.TryParse for parameterIndex and return document if that fails) while making
ParameterName optional: attempt to read DiagnosticPropertyNames.ParameterName
into parameterName if present but do not bail if it's missing or whitespace,
letting ApplyAsync use its positional fallback when parameterName is null/empty.
Update the conditional that now checks both properties to only require
parameterIndex and proceed otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec9fc70a-ea59-4c6c-9bec-8bdcdbe06631
📒 Files selected for processing (17)
src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationAnalyzer/MissingCancellationTokenAnalyzer.cssrc/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationCodeFixes/MissingCancellationTokenFixer.cssrc/ANcpLua.Roslyn.Utilities.Testing/GeneratorTestEngine.cssrc/ANcpLua.Roslyn.Utilities.Testing/LogAssert.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/DotNetSdkHelpers.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/NuGetPackageFixture.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageProjectBuilder.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/ProjectBuilder.cssrc/ANcpLua.Roslyn.Utilities.Testing/SolutionRefactoringTest.cssrc/ANcpLua.Roslyn.Utilities.Testing/Test.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/IntegrationTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/IntegrationTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities/Text/EnvConfig.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: build (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.cs
⚙️ CodeRabbit configuration file
src/**/*.cs: C# 14 / .NET 10 codebase. Review for: idiomatic modern C#, proper async/await (no sync-over-async, no fire-and-forget without justification), correct IDisposable/IAsyncDisposable, null safety (NRTs enabled), and adherence to existing patterns. Flag new public API surface that lacks XML doc comments. Check DI lifetime correctness (scoped vs singleton vs transient).
ARCHITECTURAL INVARIANTS — flag violations as blocking: 1. Every new injectable service must register OpenTelemetry instrumentation
(ActivitySource or Meter).
2. Every new DuckDB write path must handle backpressure (bounded channel or semaphore). 3. No hardcoded connection strings, paths, or magic strings — use IOptions or
IConfiguration.
4. No new dependencies on Sentry-specific types in core/ — Sentry is a comparison
target, not an identity.
5. CancellationToken must be threaded through all async public methods. 6. No sync-over-async (.Result, .GetAwaiter().GetResult()) outside of
well-documented infrastructure code.
Files:
src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationAnalyzer/MissingCancellationTokenAnalyzer.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/KestrelTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/IntegrationTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/NuGetPackageFixture.cssrc/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationCodeFixes/MissingCancellationTokenFixer.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageProjectBuilder.cssrc/ANcpLua.Roslyn.Utilities/Text/EnvConfig.cssrc/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/IntegrationTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/SolutionRefactoringTest.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/DotNetSdkHelpers.cssrc/ANcpLua.Roslyn.Utilities.Testing/GeneratorTestEngine.cssrc/ANcpLua.Roslyn.Utilities.Testing/LogAssert.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageTestBase.cssrc/ANcpLua.Roslyn.Utilities.Testing/MSBuild/ProjectBuilder.cssrc/ANcpLua.Roslyn.Utilities.Testing/Test.cs
🔇 Additional comments (26)
src/ANcpLua.Roslyn.Utilities.Testing/LogAssert.cs (4)
330-334: Looks good.
ConfigureAwait(false)is applied correctly on this awaited polling call.
354-358: Looks good.
ConfigureAwait(false)is applied correctly on this awaited polling call.
378-382: Looks good.
ConfigureAwait(false)is applied correctly on this awaited polling call.
404-404: Looks good.
ConfigureAwait(false)is applied correctly on this awaited polling call.src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageTestBase.cs (2)
299-304: LGTM:ConfigureAwait(false)is appropriate here.This keeps the testing helper aligned with the broader async context-capture sweep and does not change the build flow.
350-355: LGTM:ConfigureAwait(false)is appropriate here.Same rationale as
QuickBuild: the async flow is unchanged, and avoiding context capture is consistent for this library code.src/ANcpLua.Roslyn.Utilities.Testing/SolutionRefactoringTest.cs (4)
104-118: No concerns.
GetTextAsyncnow honors the caller's cancellation token, and the continuation handling matches the surrounding async test helpers.
164-181: No concerns.This keeps the multi-project path aligned with the rest of the sweep: cancellation is threaded through, and the awaits stay context-free.
203-218: No concerns.The refactoring lookup and no-refactoring verification paths now consistently propagate the caller's cancellation token.
288-297: No concerns.The Roslyn async calls here are updated consistently, and
ConfigureAwait(false)is applied in the right places.src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/IntegrationTestBase.cs (1)
48-48: No concerns. This teardown-onlyDisposeAsync().ConfigureAwait(false)change is consistent with the rest of the test fixture cleanup and does not alter behavior.src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/KestrelTestBase.cs (1)
53-53: No concerns. Awaiting_factory.DisposeAsync().ConfigureAwait(false)is the right teardown pattern here and keeps the NUnit fixture consistent with the other WebTesting bases.src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/NUnit/IntegrationTestBase.cs (1)
56-56: No concerns.Factory.DisposeAsync().ConfigureAwait(false)is a safe teardown adjustment and matches the disposal style used elsewhere in the PR.src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/TUnit/KestrelTestBase.cs (1)
54-54: No concerns. The awaited async dispose keeps the TUnit Kestrel teardown aligned with the rest of the fixture cleanup changes.src/ANcpLua.Roslyn.Utilities.Testing/WebTesting/KestrelTestBase.cs (1)
58-58: No concerns. Awaiting_baseFactory.DisposeAsync().ConfigureAwait(false)is a consistent cleanup improvement and does not change the test base semantics.src/ANcpLua.Roslyn.Utilities.Testing/GeneratorTestEngine.cs (1)
123-123: ConfigureAwait usage is consistent and correct for library async paths.These awaits now avoid context capture without changing control flow or cancellation behavior.
Also applies to: 166-166
src/ANcpLua.Roslyn.Utilities.Testing/Test.cs (1)
24-24: Async continuation policy is now consistently applied across test entry points.All three awaits correctly use
ConfigureAwait(false)and preserve token flow.Also applies to: 38-38, 48-48
src/ANcpLua.Roslyn.Utilities/Text/EnvConfig.cs (2)
20-24: No blockers here.The whitespace handling and trimming logic are consistent with the other readers.
34-35: No blockers here.The combined parse-and-bounds guard preserves the default-on-miss behavior.
src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationAnalyzer/MissingCancellationTokenAnalyzer.cs (1)
91-95: No concerns.src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/DotNetSdkHelpers.cs (1)
101-197: No concerns with the async continuation change.The awaits in this path now consistently avoid capturing the synchronization context, without changing the download/cache logic.
src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/PackageProjectBuilder.cs (2)
246-341: No concerns with the async execution-path update.The SDK resolution, process execution, retry delay, and artifact reads all now consistently avoid context capture.
565-571: No concerns with the git bootstrap await update.The initialization sequence now follows the same
ConfigureAwait(false)pattern as the rest of the helper.src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/ProjectBuilder.cs (2)
241-245: No concerns with the disposal-path change.
DisposeAsyncnow avoids capturing the synchronization context, which matches the rest of the async test infrastructure.
1048-1160: No concerns with the command execution and SARIF loading update.These awaits now consistently avoid context capture across the logging, process, and file-loading paths.
src/ANcpLua.Roslyn.Utilities.Testing/MSBuild/NuGetPackageFixture.cs (1)
158-249: No concerns with the fixture async-path update.Initialization, disposal, and restore now all follow the same
ConfigureAwait(false)pattern.
| if (!diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterName, out var parameterName) | ||
| || string.IsNullOrWhiteSpace(parameterName) | ||
| || !diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterIndex, out var parameterIndexText) | ||
| || !int.TryParse(parameterIndexText, out var parameterIndex)) | ||
| return document; |
There was a problem hiding this comment.
Don't block the fix when ParameterName is missing.
ApplyAsync already has a positional fallback, so requiring ParameterName up front turns a recoverable diagnostic into a no-op. Keep ParameterIndex as the hard requirement and treat ParameterName as optional for the named-argument branch.
♻️ Proposed fix
- if (!diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterName, out var parameterName)
- || string.IsNullOrWhiteSpace(parameterName)
- || !diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterIndex, out var parameterIndexText)
- || !int.TryParse(parameterIndexText, out var parameterIndex))
+ if (!diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterIndex, out var parameterIndexText)
+ || !int.TryParse(parameterIndexText, out var parameterIndex))
return document;
+
+ diagnostic.Properties.TryGetValue(DiagnosticPropertyNames.ParameterName, out var parameterName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/ANcpLua.Roslyn.Utilities.Examples.XunitCancellationCodeFixes/MissingCancellationTokenFixer.cs`
around lines 51 - 55, The current guard in MissingCancellationTokenFixer
prevents the code fix from running if DiagnosticPropertyNames.ParameterName is
absent; change the logic so that ApplyAsync only treats
DiagnosticPropertyNames.ParameterIndex as mandatory (keep the TryGetValue +
int.TryParse for parameterIndex and return document if that fails) while making
ParameterName optional: attempt to read DiagnosticPropertyNames.ParameterName
into parameterName if present but do not bail if it's missing or whitespace,
letting ApplyAsync use its positional fallback when parameterName is null/empty.
Update the conditional that now checks both properties to only require
parameterIndex and proceed otherwise.
Summary
Three thematic cleanups bundled together (all pre-existing WIP):
ConfigureAwait(false)sweep across Testing/WebTesting library —LogAssert,GeneratorTestEngine,SolutionRefactoringTest,Test, MSBuild builders/fixtures (DotNetSdkHelpers,NuGetPackageFixture,PackageProjectBuilder,PackageTestBase,ProjectBuilder), and Kestrel/Integration test bases (xUnit/NUnit/TUnit). Standard library guidance for code that ships to consumers.Examples pattern cleanups:
MissingCancellationTokenAnalyzer: nest the property pattern (Property: { Name, ContainingType }) instead of two parallelProperty.*paths.MissingCancellationTokenFixer: collapse three sequential guardifstatements into one short-circuit chain.EnvConfigsurface tightening:ReadStringfrompublictoprivate— it's an internal helper for the typedRead*methods; external callers should useEnvironment.GetEnvironmentVariabledirectly.Test plan
dotnet build -c Release— 0 warnings, 0 errorsNotes
EnvConfig.ReadStringis no longer public. Per repo policy, breaking changes are allowed; downstream consumers (qyl, Agents, Analyzers) are owner-controlled and will be patched in the same session if affected.mainand can land in either order.🤖 Generated with Claude Code