Modernize the build: BenchmarkDotNet, MinVer, one workflow, a net10.0 leg - #64
Merged
Merged
Conversation
…d another library are gone SpeedTest.cs was 28 tests timing 500,000 iterations and asserting the result was faster than reflection: red on a loaded machine, vacuous on a fast one, and filtered in CI by a category that did not match them. All 28 measured Dynamitey (Dynamic.*, CacheableInvocation, FastDynamicInvoke); none touched ActLike. Left over from the 2013 split. In its place a BenchmarkDotNet project that measures this library: creating a proxy, and reading, writing and calling through one, each against the same operation on a type that declares the interface. Those baseline accessors are NoInlining - inlined, the JIT elides them, the baseline reads 0 ns and every ratio is meaningless. The suite is green for the first time: 194 passed, 0 failed, 0 skipped. Part of #63. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
…at nuget.org Version.props carried the prefix by hand and built the suffix from GITHUB_RUN_NUMBER, so a local build and a CI build of the same commit disagreed. MinVer reads the tags instead; MinVerMinimumMajorMinor is where the next intended release is declared, 8.1 here, so untagged builds read 8.1.0-alpha.<height> rather than claiming a patch. dotnet.yml and dotnet48.yml become one build.yml over ubuntu/macOS/ windows, with the net47 leg only where the framework exists, packing and pushing a prerelease from master. Warnings are errors when CI=true. Also gone: the CopyPackage target, which resolved $(SolutionDir) to / and failed whenever the project was built on its own rather than through the solution; GeneratePackageOnBuild, since CI packs explicitly; the stale Dynamitey.myget feed and the in-repo globalPackagesFolder. The support library moves off netstandard1.0 (NETSDK1215) to netstandard2.0. Part of #63. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
NuGet hands a .NET Framework consumer the net40 asset however new their framework is, so that leg serves all of them and dropping it would break real consumers; netstandard2.0 likewise. net10.0 is purely additive - a .NET 10 consumer gets it instead of falling back to netstandard2.0. What the new leg turned up: binary serialization is obsolete from .NET 8 (SYSLIB0050/0051), and ISerializable sits on the public ActLikeProxy base that every emitted proxy derives from. Suppressed there rather than compiled out, so the public surface does not differ per target framework. Also, with warnings as errors, a critical advisory surfaced in a package the tests never call: IronPython 2.7 -> DynamicLanguageRuntime 1.3.1 -> System.Drawing.Common 4.7.0 (GHSA-rxg9-xrhp-64gj). Pinned to a patched 8.0.x. IronPython 3.4 would drop the dependency outright, but its overload resolution rejects two of the Where/Overloads forms in Linq.cs. Tests run on net8.0 and net10.0 (194 each); CI installs both SDKs. Part of #63. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
…tey — and with them a critical advisory Four of the nine measured `Dynamic.Linq` with no proxy involved (SimpleLinqDynamicLinq, MoreGenericsDynamicLinq, PythonDynamicLinq, PythonDynamicLinqGenericArgs): Dynamitey's tests living in this repo, like SpeedTest.cs was. The four that remain are ActLike over a Dynamitey Linq proxy, two of them driven from IronPython — a proxy called through a foreign DLR binder, which nothing else here covers. That unblocked IronPython 3.4, which drops DynamicLanguageRuntime 1.3.1 and with it System.Drawing.Common 4.7.0 (GHSA-rxg9-xrhp-64gj, critical) — so the pin added in the previous commit is gone rather than needed. The one test that still failed on 3.4 needed `System.Func[System.Int32, System.Boolean]` rather than `System.Func[int, bool]`: on IronPython 3 the Python `int` is arbitrary precision and no longer means Int32. 190 tests on net8.0 and net10.0; a CI-flagged build of the solution has no warnings left. Part of #63. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical CI and publishing failures, along with additional workflow and dependency issues, block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (5)
What changed in this PR
This PR modernizes versioning, benchmarking, CI, dependency management, and adds .NET 10 support.
Changes:
- Replaces flaky timing tests with BenchmarkDotNet benchmarks.
- Introduces MinVer and updates target frameworks and test dependencies.
- Consolidates CI, testing, and publishing into one workflow.
| File | Summary |
|---|---|
Version.props |
Removed for MinVer migration. Nit (1 vote): remove the dangling solution-item entry. |
Tests/UnitTestSupportLibrary/UnitTestSupportLibrary.csproj |
Retargets the support library to netstandard2.0. |
Tests/UnitTestImpromptuInterface/UnitTestImpromptuInterface.csproj |
Adds .NET 10 and IronPython 3.4. Moderate (1 vote): the Clay project still references vulnerable IronPython 2.7. Nit (1 vote): clarify the dependency explanation. |
Tests/UnitTestImpromptuInterface/Support/SupportDefinitions.cs |
Removes timing helpers. Moderate (2 votes): Clay tests still depend on TimeIt. |
Tests/UnitTestImpromptuInterface/SpeedTest.cs |
Removes obsolete wall-clock performance tests. |
Tests/UnitTestImpromptuInterface/Linq.cs |
Removes unrelated Dynamitey tests and updates retained proxy/LINQ coverage. |
NuGet.config |
Cleans package sources and cache settings. |
ImpromptuInterface/ImpromptuInterface.csproj |
Adds net10.0 packaging and serialization warning handling. |
Directory.Build.props |
Adds MinVer and CI warning policy. Nit (2 votes): remove the stale solution-item entry. |
Benchmarks/Readme.md |
Documents benchmark execution and results. |
Benchmarks/ProxyBenchmarks.cs |
Defines proxy performance benchmarks. |
Benchmarks/Program.cs |
Adds the BenchmarkDotNet runner. |
Benchmarks/Fixtures.cs |
Adds benchmark fixtures and baseline types. |
Benchmarks/Benchmarks.csproj |
Adds the BenchmarkDotNet project. |
BenchmarkDotNet.Artifacts/results/Benchmarks.ProxyBenchmarks-report.html |
Adds generated benchmark output. Nit (1 vote): ignore generated artifacts or retain only a curated snapshot. |
BenchmarkDotNet.Artifacts/results/Benchmarks.ProxyBenchmarks-report.csv |
Adds generated benchmark CSV output. |
BenchmarkDotNet.Artifacts/results/Benchmarks.ProxyBenchmarks-report-github.md |
Adds generated benchmark Markdown output. |
.github/workflows/dotnet48.yml |
Removes the superseded workflow. |
.github/workflows/dotnet.yml |
Removes the superseded workflow. |
.github/workflows/build.yml |
Adds unified cross-platform build, test, and publish jobs. Critical (2 votes): non-Windows solution builds target net40/net47; critical (2 votes): Ubuntu packing also requires .NET Framework assets. Moderate (2 votes): SDK selection is not pinned. Moderate (1 vote): publishing lacks packages: write. Moderate (1 vote): the quoted package glob will not expand. Nit (1 vote): update README badges for the renamed workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 10.0.x | ||
|
|
||
| - name: Build | ||
| run: dotnet build ImpromptuInterface.sln --configuration Release |
Comment on lines
+69
to
+70
| - name: Pack | ||
| run: dotnet pack ImpromptuInterface/ImpromptuInterface.csproj --configuration Release --output packages |
Comment on lines
+30
to
+32
| dotnet-version: | | ||
| 8.0.x | ||
| 10.0.x |
| } | ||
| } | ||
|
|
||
| public class TestForwarder:BaseForwarder |
| <Project> | ||
|
|
||
| <!-- | ||
| Version comes from git tags, through MinVer, rather than a hand-maintained Version.props. |
This was referenced Sep 22, 2026
This was referenced Sep 24, 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.



Steps 1 and 2 of #63, plus the net10.0 leg. Four commits, each self-contained.
The suite is green for the first time: 190 tests on net8.0 and on net10.0, 0 failed, 0 skipped — where master is 200 passed / 17 failed / 5 skipped. A
CI=true dotnet build --no-incrementalof the solution has no warnings and no errors.1. Benchmarks, and the wall-clock tests are gone (
1510c13)Tests/UnitTestImpromptuInterface/SpeedTest.cswas 28[Test]methods timing 500,000 iterations and assertingAssert.Less(impromptuTime, reflectionTime): red on a loaded machine, vacuous on a fast one, and "handled" in CI by aTestCategory!=Performancefilter that did not match them.All 28 measured Dynamitey —
Dynamic.InvokeSet,CacheableInvocation,FastDynamicInvoke— and not one touchedActLike. They were left over from the 2013 split, so they are deleted rather than migrated; they belong to that repo if anywhere.In their place
Benchmarks/, measuring this library: creating a proxy, and reading, writing and calling through one, each against the same operation on a type that declares the interface, so a ratio reads as what the proxy adds.ActLike<IPoco>(), type already builtMethodInfo.Invokeis ~4.4× andPropertyInfo.GetValue~10× in the same run, so a proxy call is cheaper than reflection and creating one is not. The baseline accessors areMethodImpl(NoInlining)on purpose: inlined, the JIT elides them, the baseline measures 0 ns and every ratio is meaningless — that happened on the first run.2. MinVer, one workflow, NuGet.config (
932045c)Version.propscarried the prefix by hand and built the suffix fromGITHUB_RUN_NUMBER, so a local build and a CI build of the same commit disagreed. MinVer reads the tags;MinVerMinimumMajorMinoris8.1, so untagged builds read8.1.0-alpha.<height>.dotnet.yml+dotnet48.ymlbecome onebuild.ymlacross ubuntu/macOS/windows, with the net47 leg only where the framework exists and a publish job for master. Warnings are errors whenCI=trueonly.Two bugs fixed in passing: the
CopyPackagetarget resolved$(SolutionDir)to/and failed whenever the library was built on its own rather than through the solution; andUnitTestSupportLibrarytargetednetstandard1.0(NETSDK1215).3. A net10.0 leg (
46beee2)net40;netstandard2.0;net10.0. Both existing legs stay — NuGet hands a .NET Framework consumer thenet40asset however new their framework is, so dropping it breaks real consumers. net10.0 is purely additive: a .NET 10 consumer gets it instead of falling back to netstandard2.0. Package verified to carry all threelib/folders with the right dependency group each (net10.0 gets only Dynamitey;Microsoft.CSharpandSystem.Reflection.Emitare inbox there).The new leg surfaced that binary serialization is obsolete from .NET 8 (SYSLIB0050/0051).
ISerializableis on the publicActLikeProxybase that every emitted proxy derives from, so it is suppressed on that leg rather than compiled out — the public surface should not differ per target framework.4. Linq tests, IronPython 3.4, and a critical advisory (
f2b4cf9)Same pattern as
SpeedTest.cs: four of the nine Linq tests exercisedDynamic.Linqwith no proxy involved. Deleted. The four kept areActLikeover a Dynamitey Linq proxy, two driven from IronPython — a proxy called through a foreign DLR binder, which nothing else here covers.That unblocked IronPython 3.4, which drops
DynamicLanguageRuntime1.3.1 and with itSystem.Drawing.Common4.7.0 — a critical advisory (GHSA-rxg9-xrhp-64gj) that only became visible once warnings are errors. The one test that still failed on 3.4 neededSystem.Func[System.Int32, System.Boolean]rather thanSystem.Func[int, bool]: on IronPython 3 the Pythonintis arbitrary precision and no longer meansInt32.Not in this PR
Step 3 of #63 (the AnyUnit port) is proven on a local branch but waits on AnyUnit 1.3.0 reaching nuget.org. Step 4 (
Tests.Wasm) needs that harness — though the capability question behind it is now answered: proxy emit works on browser-wasm under .NET 10, interpreted and AOT-published alike, which closed #43 and narrowed #48 to trimming.🤖 Generated with Claude Code
https://claude.ai/code/session_016mwfq4oeZW8SiD4HjTHYdg