Add representative workflow benchmark suite#70
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new BenchmarkDotNet-based project (Arius.Benchmarks) including CLI parsing, git-head resolution, tail-log appending, a representative Azurite workflow benchmark, sample raw reports, solution/package updates, an InternalsVisibleTo entry, and a CI job to run and optionally commit benchmark outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Program as Program (Runner)
participant Git as GitHeadResolver
participant BDN as BenchmarkDotNet
participant RepBench as RepresentativeWorkflowBenchmarks
participant Fixture as AzuriteE2EBackendFixture
participant Workflow as RepresentativeWorkflowRunner
participant TailLog as BenchmarkTailLog
Program->>Program: Parse CLI args, locate repo, create run dir
Program->>Git: Resolve(repo root)
Git-->>Program: commit hash or "unknown"
Program->>BDN: Run benchmarks with ManualConfig & StreamLogger
BDN->>RepBench: [GlobalSetup] SetupAsync
RepBench->>Fixture: Create & InitializeAsync
Fixture-->>RepBench: Ready
loop iterations
BDN->>RepBench: Invoke benchmark method
RepBench->>Workflow: RunAsync representative workflow
Workflow-->>RepBench: Result
RepBench->>RepBench: Validate result (skip/archive checks)
end
BDN->>RepBench: [GlobalCleanup] CleanupAsync
RepBench->>Fixture: DisposeAsync
Program->>TailLog: Append(run metadata, summary, raw path)
TailLog-->>TailLog: Write header if missing and append pipe-delimited row
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
README.md (1)
155-155: Trim implementation-heavy wording in the benchmark section.Line 155 is a bit too internal-path/tooling heavy for README flow; consider keeping this section focused on how to run and where to look next, with less implementation detail.
✍️ Suggested wording
-The benchmark runs the canonical representative workflow with BenchmarkDotNet. Raw BenchmarkDotNet output is written under `src/Arius.Benchmarks/raw/`, and each run appends one line to `src/Arius.Benchmarks/benchmark-tail.log` for autoresearch-style tailing. +This runs the canonical representative workflow benchmark. Results are saved under `src/Arius.Benchmarks/raw/`, and a summary line is appended to `src/Arius.Benchmarks/benchmark-tail.log`.As per coding guidelines:
README.mdshould stay high signal and accessible, and should not be cluttered with implementation details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 155, Replace the implementation-heavy sentence about BenchmarkDotNet output with a concise, user-focused line: remove internal paths and tooling specifics and instead state how to run the benchmark and where to inspect results (e.g., “Run the benchmark with BenchmarkDotNet; raw output and logs are available in the benchmarks folder”); update the README's benchmark section to be high-level and actionable, keeping phrasing similar to the existing sentence but omitting `src/Arius.Benchmarks/raw/` and `src/Arius.Benchmarks/benchmark-tail.log` details.src/Arius.E2E.Tests/AssemblyMarker.cs (1)
3-3: MoveSyntheticRepositoryDefinitionFactory, E2E fixtures, and workflows toArius.Tests.Sharedto eliminate this broadInternalsVisibleTogrant.
Arius.Benchmarkscurrently importsArius.E2E.Tests.Datasets,Arius.E2E.Tests.Fixtures, andArius.E2E.Tests.Workflowsas internal dependencies. Extract the synthetic repository generation and fixture infrastructure intosrc/Arius.Tests.Shared/, keepingArius.E2E.Testsreserved for actual end-to-end behavior coverage. This removes the need for the friend-assembly grant and consolidates reusable test infrastructure per architectural guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.E2E.Tests/AssemblyMarker.cs` at line 3, The project currently exposes internals to Arius.Benchmarks via the AssemblyMarker InternalsVisibleTo attribute; move SyntheticRepositoryDefinitionFactory, all E2E fixtures (Arius.E2E.Tests.Fixtures) and workflows (Arius.E2E.Tests.Workflows) into a new shared test library Arius.Tests.Shared and update their namespaces so they are compiled into that project, then remove the InternalsVisibleTo("Arius.Benchmarks") attribute from AssemblyMarker.cs and update any references in Arius.Benchmarks to depend on Arius.Tests.Shared instead; specifically, extract the implementation of SyntheticRepositoryDefinitionFactory and the fixture/workflow classes into src/Arius.Tests.Shared, make them public if needed, add a project reference from Arius.Benchmarks to Arius.Tests.Shared, and remove the InternalsVisibleTo attribute to eliminate the broad friend-assembly grant.src/Arius.Benchmarks/Arius.Benchmarks.csproj (1)
16-18: Decouple benchmark runtime code fromArius.E2E.Testsassembly.
Arius.Benchmarkscurrently depends on the E2E test assembly directly. Consider moving reusable workflow/fixture primitives intosrc/Arius.Tests.Shared/and referencing that shared project from both benchmarks and E2E tests to avoid cross-assembly test coupling.Based on learnings: Reusable Azurite and repository-fixture wiring belongs in
src/Arius.Tests.Shared/, not in another test project assembly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Benchmarks/Arius.Benchmarks.csproj` around lines 16 - 18, Arius.Benchmarks currently references the test assembly Arius.E2E.Tests; extract the reusable fixture/workflow primitives (e.g., Azurite wiring, repository fixtures) into a new or existing shared project src/Arius.Tests.Shared and update usages to the shared namespace, then remove the ProjectReference to ..\Arius.E2E.Tests\Arius.E2E.Tests.csproj from Arius.Benchmarks.csproj and add a ProjectReference to the new src/Arius.Tests.Shared project; also update Arius.E2E.Tests to reference Arius.Tests.Shared so both benchmarks and E2E tests consume the same shared primitives without cross-assembling coupling.src/Arius.Benchmarks/BenchmarkTailLog.cs (1)
78-105: Split extra top-level types into dedicated files.
BenchmarkTailLogEntryandBenchmarkSummaryValuesshould be moved out ofBenchmarkTailLog.csso each top-level type has a matching file.As per coding guidelines: "Prefer one top-level class per file, with the filename matching the class name."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Arius.Benchmarks/BenchmarkTailLog.cs` around lines 78 - 105, Move the two top-level types out of BenchmarkTailLog.cs into their own files: create a file that declares the internal sealed record BenchmarkTailLogEntry with the same signature and accessibility, and another file that declares the internal sealed class BenchmarkSummaryValues with the same constructor, Get method, Normalize helper and accessibility; keep their namespace and any required using directives, remove these type declarations from BenchmarkTailLog.cs, and ensure any references to BenchmarkTailLogEntry or BenchmarkSummaryValues compile unchanged.
🤖 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/Arius.Benchmarks/GitHeadResolver.cs`:
- Around line 9-25: The current GitHeadResolver logic can throw when
Process.Start fails, block indefinitely on process.WaitForExit(), and never
consumes StandardError; update the method that creates the Process (using
ProcessStartInfo and the local variable process) to wrap Process.Start in a
try/catch and immediately return "unknown" on exception, set a bounded wait via
process.WaitForExit(timeoutMs) (e.g. 5s), on timeout call process.Kill() and
return "unknown", and always read both process.StandardOutput and
process.StandardError (or drain StandardError) before checking process.ExitCode
to avoid deadlocks; ensure the process is disposed in a finally block so
repositoryRoot resolution always fails closed to "unknown".
In
`@src/Arius.Benchmarks/raw/20260428T040505.928Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.md`:
- Around line 1-15: Add a fenced language to the opening code fence (change the
initial ``` to ```text) and ensure there is a blank line before the markdown
table header row ("| Method ...") and a blank line after the table end, so the
initial code block is properly language-marked (fixes MD040) and the table is
separated by surrounding blank lines (fixes MD058); update the file's opening
``` and the area around the table header/footer accordingly.
In `@src/Arius.Benchmarks/RepresentativeWorkflowBenchmarks.cs`:
- Line 9: The class RepresentativeWorkflowBenchmarks is declared public but
doesn't require cross-assembly access; change its declaration to internal to
narrow visibility. Update the type declaration for
RepresentativeWorkflowBenchmarks from public to internal (keeping the class name
and members unchanged) so it follows the guideline to make non-test classes
internal unless needed externally.
---
Nitpick comments:
In `@README.md`:
- Line 155: Replace the implementation-heavy sentence about BenchmarkDotNet
output with a concise, user-focused line: remove internal paths and tooling
specifics and instead state how to run the benchmark and where to inspect
results (e.g., “Run the benchmark with BenchmarkDotNet; raw output and logs are
available in the benchmarks folder”); update the README's benchmark section to
be high-level and actionable, keeping phrasing similar to the existing sentence
but omitting `src/Arius.Benchmarks/raw/` and
`src/Arius.Benchmarks/benchmark-tail.log` details.
In `@src/Arius.Benchmarks/Arius.Benchmarks.csproj`:
- Around line 16-18: Arius.Benchmarks currently references the test assembly
Arius.E2E.Tests; extract the reusable fixture/workflow primitives (e.g., Azurite
wiring, repository fixtures) into a new or existing shared project
src/Arius.Tests.Shared and update usages to the shared namespace, then remove
the ProjectReference to ..\Arius.E2E.Tests\Arius.E2E.Tests.csproj from
Arius.Benchmarks.csproj and add a ProjectReference to the new
src/Arius.Tests.Shared project; also update Arius.E2E.Tests to reference
Arius.Tests.Shared so both benchmarks and E2E tests consume the same shared
primitives without cross-assembling coupling.
In `@src/Arius.Benchmarks/BenchmarkTailLog.cs`:
- Around line 78-105: Move the two top-level types out of BenchmarkTailLog.cs
into their own files: create a file that declares the internal sealed record
BenchmarkTailLogEntry with the same signature and accessibility, and another
file that declares the internal sealed class BenchmarkSummaryValues with the
same constructor, Get method, Normalize helper and accessibility; keep their
namespace and any required using directives, remove these type declarations from
BenchmarkTailLog.cs, and ensure any references to BenchmarkTailLogEntry or
BenchmarkSummaryValues compile unchanged.
In `@src/Arius.E2E.Tests/AssemblyMarker.cs`:
- Line 3: The project currently exposes internals to Arius.Benchmarks via the
AssemblyMarker InternalsVisibleTo attribute; move
SyntheticRepositoryDefinitionFactory, all E2E fixtures
(Arius.E2E.Tests.Fixtures) and workflows (Arius.E2E.Tests.Workflows) into a new
shared test library Arius.Tests.Shared and update their namespaces so they are
compiled into that project, then remove the
InternalsVisibleTo("Arius.Benchmarks") attribute from AssemblyMarker.cs and
update any references in Arius.Benchmarks to depend on Arius.Tests.Shared
instead; specifically, extract the implementation of
SyntheticRepositoryDefinitionFactory and the fixture/workflow classes into
src/Arius.Tests.Shared, make them public if needed, add a project reference from
Arius.Benchmarks to Arius.Tests.Shared, and remove the InternalsVisibleTo
attribute to eliminate the broad friend-assembly grant.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ed0149e-5aa0-455f-b88e-ce0aa1debb3f
⛔ Files ignored due to path filters (4)
src/Arius.Benchmarks/benchmark-tail.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T040505.928Z/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-20260428-060506.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T040505.928Z/benchmark-output.logis excluded by!**/*.logsrc/Arius.Benchmarks/raw/20260428T040505.928Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
.gitignoreREADME.mdsrc/Arius.Benchmarks/Arius.Benchmarks.csprojsrc/Arius.Benchmarks/BenchmarkRunOptions.cssrc/Arius.Benchmarks/BenchmarkTailLog.cssrc/Arius.Benchmarks/GitHeadResolver.cssrc/Arius.Benchmarks/Program.cssrc/Arius.Benchmarks/RepresentativeWorkflowBenchmarks.cssrc/Arius.Benchmarks/raw/20260428T040505.928Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report-github.mdsrc/Arius.Benchmarks/raw/20260428T040505.928Z/results/Arius.Benchmarks.RepresentativeWorkflowBenchmarks-report.htmlsrc/Arius.E2E.Tests/AssemblyMarker.cssrc/Arius.E2E.Tests/Datasets/SyntheticRepositoryDefinitionFactory.cssrc/Arius.slnxsrc/Directory.Packages.props
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70 +/- ##
==========================================
- Coverage 79.20% 78.85% -0.36%
==========================================
Files 69 66 -3
Lines 4986 4842 -144
Branches 671 656 -15
==========================================
- Hits 3949 3818 -131
+ Misses 881 872 -9
+ Partials 156 152 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This was referenced Apr 28, 2026
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
1 participant
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.
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
CI
Chores