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 12 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.