You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a packaged trimmed-publish smoke test to the existing Ubuntu job that builds and packs libraries for documentation validation. The script consumes the freshly produced TUnit.Core package from an isolated NuGet cache, checks that the linker descriptor sits beside every targets file in both build layouts, and publishes self-contained, non-AOT net10.0 consumers.
F# workflows must execute an asynchronous side effect and propagate an asynchronous exception with both normal and runtime-only (ExcludeAssets="compile") FSharp.Core references. A C# consumer without FSharp.Core must publish without warnings. A negative control removes only the preservation descriptor and must fail specifically with ASYNC_BODY_SKIPPED, proving the execution check detects the original regression. CI uploads publish and execution logs.
Validation: built and packed TUnit.Core and its analyzer dependencies with SDK 10.0.403 on macOS arm64, then ran pwsh scripts/Verify-FSharpTrimming.ps1 -PackagesPath artifacts/trim-packages -Version 99.99.99. All four scenarios passed, including the expected negative-control failure. FSharp.Core publishes retain the known IL2040 resource warnings; C# emitted none. Existing analyzer/code-fixer build warnings were confirmed on #7410. Ubuntu execution will be validated by CI after draft promotion.
Summary by CodeRabbit
Tests
Added automated validation for trimmed .NET packages across F# and C# consumer scenarios.
Checks now cover F# asynchronous success and exception handling, package descriptors, and publish warnings.
A negative-control scenario verifies that the checks detect when required preservation metadata is absent.
Verification logs are retained as workflow artifacts for review when diagnosing failures.
No actionable comments were generated in the recent review.
Walkthrough
The doc-snippets workflow job now runs a script that checks packaged F# async trimming behavior across several trimmed consumers. The job also uploads the script’s logs as an artifact.
Changes: Packaged F# trimming verification
Layer / File(s)
Summary
Package checks and test harness setup scripts/Verify-FSharpTrimming.ps1
The script checks for build and buildTransitive targets and their F# descriptors. It creates isolated NuGet and MSBuild settings and builds an F# test library.
The script defines compile, runtime-only, no-fsharp, and disabled consumers. The F# scenarios check async execution and exception propagation. The disabled scenario removes the F# trimmer root descriptor.
Publish checks and CI integration scripts/Verify-FSharpTrimming.ps1, .github/workflows/dotnet.yml
The script publishes and runs the consumers, checks their output, and saves logs. The workflow runs the script and always uploads matching logs as an artifact.
sequenceDiagram
participant DocSnippets as doc-snippets job
participant Verify as Verify-FSharpTrimming.ps1
participant Package as TUnit.Core package
participant Consumers as trimmed consumers
DocSnippets->>Verify: run package verification
Verify->>Package: inspect targets and F# descriptors
Verify->>Consumers: publish and run scenarios
DocSnippets->>DocSnippets: upload trimming logs
Loading
Possibly related issues
thomhurst/TUnit#7523 — The script checks a C# publish without FSharp.Core for warnings, but this PR does not address the issue’s Native AOT warning.
Possibly related PRs
thomhurst/TUnit#7526 — This PR checks the packaged descriptor and preservation behavior introduced by that PR.
Merge Risk: Minimal · up to e333c
The change adds packaged trimming smoke coverage. No merge-blocking issue was identified.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Add packaged F# async trimming smoke coverage" matches the main change. The PR adds a packaged trimmed-publish smoke test (scripts/Verify-FSharpTrimming.ps1) and a CI step in the doc-snippe…
Linked Issues check
Issue #7533 requires packaged trimmed non-AOT coverage for F# async execution. The workflow runs Verify-FSharpTrimming.ps1 after package production. The script inspects the TUnit.Core archive for …
Out of Scope Changes check
The changed workflow steps and Verify-FSharpTrimming.ps1 script directly implement the automated packaged trimming coverage requested by issue #7533. Log upload, isolated restore setup, and the nega…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Title check
Explanation
The title "Add packaged F# async trimming smoke coverage" matches the main change. The PR adds a packaged trimmed-publish smoke test (scripts/Verify-FSharpTrimming.ps1) and a CI step in the doc-snippets job. The test covers F# async execution and exception propagation in trimmed, self-contained consumers of the TUnit.Core package. The title is short, specific, and uses no vague terms.
Full details: Linked Issues check
Explanation
Issue #7533 requires packaged trimmed non-AOT coverage for F# async execution. The workflow runs Verify-FSharpTrimming.ps1 after package production. The script inspects the TUnit.Core archive for targets and matching descriptors in both build and buildTransitive, isolates package restore, and publishes self-contained net10.0 consumers. It covers F# async side effects and exception propagation with normal and ExcludeAssets="compile"FSharp.Core references. It also checks a C# consumer without FSharp.Core for warnings and runs a descriptor-removal negative control that requires ASYNC_BODY_SKIPPED. The workflow uploads the scenario logs.
Full details: Out of Scope Changes check
Explanation
The changed workflow steps and Verify-FSharpTrimming.ps1 script directly implement the automated packaged trimming coverage requested by issue #7533. Log upload, isolated restore setup, and the negative control support that coverage. No unrelated change is established by the reviewed diff.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each package trail Through trimmed builds, the tests prevail Async steps run, exceptions show The disabled path confirms the flow Logs hop safely to CI’s store
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 6b5e8089-e5b9-4966-bb70-92f1ea34e2c5
Commits
Reviewing files that changed from the base of the PR and between be37704 and e333c0d.
Files selected for processing: 2
.github/workflows/dotnet.yml
scripts/Verify-FSharpTrimming.ps1
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
Solid addition. The script checks the packaged archive, isolates consumers from repo build settings, and covers the normal, runtime-only, no-FSharp.Core and negative-control scenarios. The negative control is the strongest part: it proves the check can actually fail with ASYNC_BODY_SKIPPED.
Minor suggestions, none blocking:
Fail fast on a missing FSharp.Core version. If Directory.Packages.props ever stops listing FSharp.Core, $fsharpVersion becomes $null. The generated projects then fail with a confusing restore error. Add if (!$fsharpVersion) { throw ... } right after the lookup.
Anchor the target-name coupling. The negative control hooks AfterTargets="_TUnitPreserveFSharpAsync". If that private target is renamed, the AfterTargets hook silently never runs, the descriptor stays in place, and the control fails with an unrelated message (it expects ASYNC_BODY_SKIPPED). That is still a failure, so it is safe. A clearer error would be to assert the descriptor is actually removed, for example by checking the publish log or the item list.
Escape the regex dots. In '^build(Transitive)?/.*/TUnit.Core.targets$' the dots are unescaped. This is harmless but imprecise.
Add Add-Type -AssemblyName System.IO.Compression.FileSystem before using [IO.Compression.ZipFile]. It is not needed on pwsh 7, but it is cheap insurance if the script is ever run under Windows PowerShell 5.
Check how to pin NuGet sources. The nuget.org source is live, so CI depends on network access. That is acceptable, but FSharp.Core could be pinned and cached to cut flakiness.
Overall this looks good to merge once CI on Ubuntu confirms the run.
Disposition of Claude’s nonblocking suggestions on 274c521:
Keep the current FSharp.Core version lookup for this PR; the checked-in central version supplies it and restore fails if it is removed.
Keep the current negative-control hook: a target rename makes the required ASYNC_BODY_SKIPPED assertion fail, so it cannot silently pass.
Leave the archive regex precision nit unchanged in this fix; actual packaged target names are checked along with their corresponding descriptors.
No Windows PowerShell 5 compatibility change: the script uses pwsh and CI runs PowerShell 7.
FSharp.Core is already pinned to the central package version; the live NuGet source is an accepted restore dependency for these isolated consumers.
Greptile’s blocking exit-code finding is fixed in e333c0d and its thread is resolved. PowerShell syntax and focused success/failure exit checks passed; full packaged publish remains covered by the prior validation and CI.
Nice addition. It checks packaging (the descriptor sits next to every targets file) and runtime behaviour (async body runs, exceptions propagate). The disabled negative control fails specifically on ASYNC_BODY_SKIPPED, so the check can't pass vacuously. Isolating the consumers from repo props, central package versions and the NuGet cache is done well. I couldn't run it. I only read the diff and checked that the _TUnitPreserveFSharpAsync target and TUnit.Core.FSharp.xml pack items exist.
Suggestions (none blocking):
Negative control depends on a target name.AfterTargets="_TUnitPreserveFSharpAsync" ties the script to an internal target name. If it is renamed, the Remove hook silently never runs. The control would then fail with a passing exit code and not the expected ASYNC_BODY_SKIPPED, so the script would still fail loudly. That makes it fail-safe, but a clearer message would help. Consider asserting in the disabled branch that the failure is the expected one, as you already do, and noting the coupling in a comment.
Fail early if no targets are found. If the regex ^build(Transitive)?/.*/TUnit.Core.targets$ matches nothing, the -like checks already throw, which is fine. A more explicit message listing the entries found would make the failure easier to diagnose if the pack layout changes (for example, targets directly under build/, which this regex wouldn't match).
[IO.Compression.ZipFile]. This works in PowerShell 7 (the CI default). Adding Add-Type -AssemblyName System.IO.Compression.FileSystem would make the script safe under Windows PowerShell 5.1 too, which is cheap.
Network dependence. The consumers restore FSharp.Core from nuget.org. This is fine for CI, but Workflows.fsproj is built with the default SDK settings. Pinning the F# SDK behaviour is unnecessary, so just be aware this is a flake source.
Job placement. The step runs in doc-snippets, so it only runs when the PR is not a draft, and it shares the 1.0.0-docs pack. That is pragmatic. If the job gets renamed or split later, this check is easy to lose, so a comment above the step or a rename like packaged-consumer-checks would help.
Exit code. The trailing exit 0 is correct given the negative control. It's cleaner to reset with $global:LASTEXITCODE = 0 so the script is not masking real failures later if steps are added. Errors currently throw, so it's fine as is.
Overall this looks good to merge once CI on Ubuntu confirms the four scenarios pass.
Reviewed all review surfaces for e333c0d. Codex reports no findings and Claude explicitly reports no blockers. Disposition of the latest Claude suggestions: retain the fail-safe negative control and existing packaging failure message; neither can silently pass when its assumption changes. PowerShell 7 is the supported CI runtime, so Windows PowerShell compatibility is unnecessary. The central FSharp.Core version is already pinned; network restore is expected. Keep the check in the existing package-producing documentation job and retain the explicit final exit 0: all unexpected failures throw before it. These are nonblocking clarity suggestions and require no further code change.
Promoting this reviewed head to ready so Ubuntu and the remaining CI can validate it. Auto-merge will be considered after the ready-for-review Claude run completes.
This is a solid regression guard for #7533. It checks the packaged descriptor in the nupkg, not the source pack items. It covers both FSharp.Core reference modes, and a C# consumer without FSharp.Core. The negative control is the strongest part: it fails specifically on ASYNC_BODY_SKIPPED, and it also fails safely if _TUnitPreserveFSharpAsync is ever renamed, because the descriptor would then not be removed and the control would report that it did not detect the skip. I did not run the script. My review is from reading the diff.
Non-blocking suggestions:
Tighten the target regex.TUnit.Core.targets uses unescaped dots. Escape them as TUnit\.Core\.targets. The /.*/ also requires a subfolder under build/, so a targets file placed directly in build/ would not be found. If that layout can exist, it will be reported as missing rather than checked, and the check would be silently weaker than intended.
CI flakiness from the network. Each scenario restores FSharp.Core and the runtime packs for self-contained trimming from nuget.org. The step could fail on an outage, not on a real regression. This is probably acceptable. If it becomes a problem, a retry on restore would help.
Maintainability. The script embeds about 100 lines of project, F# and C# text in here-strings. Moving the fixtures to scripts/fsharp-trimming/ and copying them would make them lintable and easier to edit. It is not required.
Explicit Add-Type.[IO.Compression.ZipFile] works in pwsh 7 on the runner. Add Add-Type -AssemblyName System.IO.Compression.FileSystem if anyone runs this under Windows PowerShell.
Overall this is good to merge once CI on Ubuntu passes, since the author notes that run has not happened yet.
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
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
Closes #7533
Adds a packaged trimmed-publish smoke test to the existing Ubuntu job that builds and packs libraries for documentation validation. The script consumes the freshly produced TUnit.Core package from an isolated NuGet cache, checks that the linker descriptor sits beside every targets file in both build layouts, and publishes self-contained, non-AOT net10.0 consumers.
F# workflows must execute an asynchronous side effect and propagate an asynchronous exception with both normal and runtime-only (
ExcludeAssets="compile") FSharp.Core references. A C# consumer without FSharp.Core must publish without warnings. A negative control removes only the preservation descriptor and must fail specifically withASYNC_BODY_SKIPPED, proving the execution check detects the original regression. CI uploads publish and execution logs.Validation: built and packed TUnit.Core and its analyzer dependencies with SDK 10.0.403 on macOS arm64, then ran
pwsh scripts/Verify-FSharpTrimming.ps1 -PackagesPath artifacts/trim-packages -Version 99.99.99. All four scenarios passed, including the expected negative-control failure. FSharp.Core publishes retain the known IL2040 resource warnings; C# emitted none. Existing analyzer/code-fixer build warnings were confirmed on #7410. Ubuntu execution will be validated by CI after draft promotion.Summary by CodeRabbit