Conversation
…mparing TUnit.Mocks against Moq, NSubstitute, and FakeItEasy Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/dc41f1a0-84fc-4e59-9172-30c841e9b52b Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
…ock benchmarks Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/dc41f1a0-84fc-4e59-9172-30c841e9b52b Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
| environment: ${{ github.ref == 'refs/heads/main' && 'Production' || 'Pull Requests' }} | ||
| strategy: | ||
| matrix: | ||
| benchmark: [MockCreationBenchmarks, SetupBenchmarks, InvocationBenchmarks, VerificationBenchmarks, CallbackBenchmarks, CombinedWorkflowBenchmarks] | ||
| fail-fast: false | ||
| runs-on: ubuntu-latest | ||
| concurrency: | ||
| group: "mock-benchmarks-${{ matrix.benchmark }}" | ||
| cancel-in-progress: true | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Setup .NET | ||
| uses: actions/setup-dotnet@v5 | ||
| with: | ||
| dotnet-version: 10.0.x | ||
|
|
||
| - name: Run Benchmark | ||
| run: dotnet run -c Release -- --filter "*${{ matrix.benchmark }}*" --exporters json github | ||
| working-directory: "TUnit.Mocks.Benchmarks" | ||
|
|
||
| - name: Upload Results | ||
| uses: actions/upload-artifact@v7 | ||
| if: always() | ||
| with: | ||
| name: mock_benchmark_${{ matrix.benchmark }} | ||
| path: | | ||
| **/BenchmarkDotNet.Artifacts/** | ||
|
|
||
| process-and-upload-mock-benchmarks: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, fix this by explicitly specifying minimal permissions for the affected job or at the workflow root. For this workflow, the job flagged by CodeQL is run-mock-benchmarks. It only needs to read the repository contents and upload artifacts; it does not create or modify any repository data (no pushes, issues, or PRs). Therefore the safest, non-breaking change is to add permissions: contents: read to that job. The second job, process-and-upload-mock-benchmarks, already has a permissions block and does not need changes.
Concretely:
- Edit
.github/workflows/mock-benchmarks.yml. - Under
run-mock-benchmarks:(around line 9), add apermissions:block withcontents: read, aligned with the job keys. - Leave
process-and-upload-mock-benchmarksas-is.
No additional methods, imports, or dependencies are required; this is purely a YAML configuration change.
| @@ -7,6 +7,8 @@ | ||
|
|
||
| jobs: | ||
| run-mock-benchmarks: | ||
| permissions: | ||
| contents: read | ||
| environment: ${{ github.ref == 'refs/heads/main' && 'Production' || 'Pull Requests' }} | ||
| strategy: | ||
| matrix: |
Agent-Logs-Url: https://github.com/thomhurst/TUnit/sessions/dc41f1a0-84fc-4e59-9172-30c841e9b52b Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
There was a problem hiding this comment.
Code Review: Add mock library benchmarks (TUnit.Mocks vs Moq, NSubstitute, FakeItEasy)
This PR adds BenchmarkDotNet comparison benchmarks and a CI workflow for TUnit.Mocks. The overall structure is sensible and follows existing patterns in the repo (cf. speed-comparison.yml). Below are the issues I found, from most to least critical.
Critical / Correctness Issues
1. Unfair benchmark comparisons in VerificationBenchmarks and CallbackBenchmarks
Several TUnit.Mocks benchmarks do not correctly replicate what the competing libraries do.
In VerificationBenchmarks.TUnitMocks_VerifyNever, no mock object is ever invoked before asserting "never called" — which is correct — but in Moq_VerifyNever the equivalent is written without a .Setup() call first. Moq will still verify on an un-setup method (it tracks all calls implicitly), so the behaviour matches. However the asymmetry is fragile and will confuse readers who try to understand why the Moq version is faster/slower.
More seriously, in CallbackBenchmarks.TUnitMocks_CallbackWithArgs the callback captures a constant string "logged" rather than actually capturing the argument:
// TUnit.Mocks — captures a constant, does NOT read arguments
mock.Log(TUnitArg.Any<string>(), TUnitArg.Any<string>())
.Callback(() => lastMessage = "logged"); // <-- argument not used
// Moq — correctly captures the second argument
mock.Setup(x => x.Log(It.IsAny<string>(), It.IsAny<string>()))
.Callback<string, string>((level, msg) => lastMessage = msg);This means the TUnit.Mocks "with args" benchmark does less work than its competitors, making it appear faster. This directly undermines the credibility of the benchmark. Either TUnit.Mocks needs a way to receive typed callback arguments (and the benchmark should use it), or the benchmark should be labelled differently and not compared against "with args" variants.
2. Auto-merge of CI-generated PRs using --admin
gh pr merge ${{ steps.create_pr.outputs.pull-request-number }} --squash --delete-branch --adminUsing --admin bypasses required status checks and branch protection rules. If a benchmark run produces corrupt or manipulated data (e.g. via a supply-chain attack on a dependency), it will be merged to main without review. The existing speed-comparison.yml does not use --admin. Either remove the flag and let the normal merge queue handle it, or add a required manual approval step before merging.
3. actions/checkout@v6 and actions/download-artifact@v8 — non-existent versions
At time of writing the latest released versions are actions/checkout@v4, actions/upload-artifact@v4, actions/download-artifact@v4, actions/setup-dotnet@v4, actions/setup-node@v4. The workflow references @v6, @v7, and @v8 which do not exist. The workflow will fail on its first run. The existing speed-comparison.yml correctly uses @v6/@v7/@v8 — please verify whether these are internal forks or mirrors before using them; if they are, a comment explaining this would help maintainers.
Design / Maintainability Issues
4. Hardcoded per-category artifact upload steps (DRY violation)
The process-and-upload-mock-benchmarks job has six identical "Upload X Benchmark" steps that differ only in the category name. When a new benchmark class is added, six places need to be updated (the matrix, the JS categoryMap, the upload steps, the PR body, and the placeholder docs). A single upload step with a glob pattern (docs/docs/benchmarks/mocks/** / docs/static/benchmarks/mocks/**) would make the workflow self-extending:
- name: Upload All Benchmark Artifacts
uses: actions/upload-artifact@v4
if: always()
with:
name: mock-benchmark-all
path: |
docs/docs/benchmarks/mocks/
docs/static/benchmarks/mocks/
retention-days: 905. Mermaid xychart-beta chart mixes all libraries in a single bar series
In process-mock-benchmarks.js the generated Mermaid chart uses one bar array containing measurements for all four libraries side-by-side without labelling which bar belongs to which library:
bar [${data.map(d => parseMeanValue(d.Mean)).join(', ')}]xychart-beta does not support a legend when only a single bar series is declared this way. Readers cannot tell which bar is TUnit.Mocks vs Moq. Consider using separate bar entries named per-library, or switching to a pie or table-only presentation until Mermaid's xychart supports grouped bars properly.
6. parseMeanValue silently returns 0 for unrecognised input
function parseMeanValue(meanStr) {
if (!meanStr) return 0;
const cleaned = meanStr.replace(/,/g, '');
const match = cleaned.match(/[\d.]+/);
return match ? parseFloat(match[0]) : 0;
}A return value of 0 is indistinguishable from a genuine zero measurement and will silently corrupt chart data. The function should throw (or at minimum console.warn) when it cannot parse a value, so a broken BenchmarkDotNet output format doesn't produce a misleading chart.
7. Unit normalisation is incomplete
getUnit checks for μs/us but BenchmarkDotNet can also output ns, ms, s, and mixed-unit tables where individual rows use different units. The chart renders all values on the same y-axis using a single unit derived from the first row (data[0]?.Mean), which will produce wildly incorrect charts if rows use different units (e.g. TUnit.Mocks in ns vs Moq in μs).
8. BenchmarkDotNet package version is unpinned
Directory.Packages.props already contains a BenchmarkDotNet entry (used by the existing speed-comparison benchmarks). The new .csproj references BenchmarkDotNet and BenchmarkDotNet.Annotations without version overrides, so they inherit the central version — this is correct, but it should be verified that the central version is recent enough to support [SimpleJob] and the JSON/GitHub exporters used here.
9. TUnit.Mocks.Generated global using may not be portable
global using TUnit.Mocks.Generated;This namespace is emitted by the source generator for each consuming project. If the source generator is not running (e.g. a build that skips analyzers, or a CI step that only restores without building), the project will fail to compile rather than giving a clear error. A comment explaining the dependency would help.
10. Missing [Baseline] attribute
BenchmarkDotNet supports marking one benchmark as the baseline with [Benchmark(Baseline = true)], which produces a "Ratio" column showing relative performance. Since the point of these benchmarks is to show TUnit.Mocks is faster, adding Baseline = true to the TUnit.Mocks methods would make the results immediately readable without needing the Mermaid chart.
Minor / Nits
- The
process-mock-benchmarks.jsscript contains duplicateconsole.log('... Processing mock benchmark results...')calls (lines 14 and 124) — one should be removed. - Placeholder JSON files committed to
docs/static/benchmarks/mocks/(latest.json,summary.json) use the string"placeholder"for timestamps; these will appear verbatim in the docs until the first CI run. Consider using an ISO timestamp or omitting the files and having the script create them on first run. - The CI workflow only runs when
github.ref == 'refs/heads/main'for the post-processing job, but the schedule trigger always fires regardless of branch context. This means a scheduled run on a non-main branch will run all six benchmarks and then skip the post-processing silently — this is likely intentional but worth a comment. new List<User>()inSetupBenchmarksshould use[](collection expression) per the project's "modern C#" guideline.
Summary
The most important issues to address before merging are (1) the unfair callback-with-args benchmark, (2) the --admin auto-merge, and (3) verifying the action versions. The DRY violation and chart correctness issues (4–5) are also worth fixing before this becomes a long-lived daily workflow.
Adds BenchmarkDotNet benchmarks comparing TUnit.Mocks (source-generated) against Moq, NSubstitute, and FakeItEasy (runtime proxy), with CI automation and docs integration.
Benchmark project (
TUnit.Mocks.Benchmarks)Six benchmark classes covering common mocking operations:
Uses
TUnitArg/NSubArgglobal aliases to disambiguateTUnit.Mocks.Arguments.ArgfromNSubstitute.Arg.CI workflow (
.github/workflows/mock-benchmarks.yml)workflow_dispatch.github/scripts/process-mock-benchmarks.js→ generates markdown pages with Mermaid charts + JSON datamainFollows the same pattern as the existing
speed-comparison.ymlworkflow.Documentation
docs/docs/benchmarks/mocks/(auto-discovered by existingautogeneratedsidebar config)docs/static/benchmarks/mocks/— populated automatically after first workflow runDependencies
Moq 4.20.72andFakeItEasy 9.0.1toDirectory.Packages.props(NSubstitute 5.3.0already present)⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.