Add end-to-end rspec harness + benchmark analysis library - #2
Conversation
Add a second Zeitwerk loader for spec/support (namespace: Ready) and exclude :e2e-tagged specs from the fast loop unless READY_E2E is set.
Builds an isolated ready runtime in a temp prefix, exposes the env a pty needs to attach to the live by-server, and tears it down (by-server stop while the socket is present, then reap by argv, then rm the tree). Drops a stale GEM_HOME/GEM_PATH left by a removed version manager so the build resolves gems from the project bundle.
Grounded in a measured per-layer investigation (irb, CLOCK_REALTIME both sides). Additive design: instrumented copies on both arms, zero gem-runtime changes, reusing Ready::Sandbox/Executable/Bench. rake bench + :e2e guard.
Replace the kamal-style SPANS with the ready launch layers (shell, rbenv_shim, rubygems, dep_activate, tool_run, reap, dispatch_infra, full); add Aggregator (min for process spans, median for in-process); retarget Report to `full`.
Dev-only fixtures; exclude bench/ and docs/ from the packaged gem.
Refine SPANS to launch/rubygems/dep_activate/tool_run (split Gem.activate_bin_path from the tool require via a stub copy). Validated on the real irb stub: launch 71 · dep_activate 34 · tool_run 94 · full 207 ms.
Injects server_entry + pre_tool marks into the real Ready::Executable render output; builds a faithful ready_<exe> function that dispatches through ready_by.
Builds a warm sandbox (gems: preload), runs cold (instrumented copies) and hot (ready dispatch) arms interleaved from a bundler-free shell, aggregates min/median, derives the rbenv-shim addendum. Add gems: to Ready::Sandbox. Measured: irb hot ~30ms vs cold ~258ms (~8x) on this container.
Structural, non-flaky: asserts cold pays dep_activate + tool_run, hot full << cold full, and hot skips the rbenv/boot layer. Reduced cold arm (no rbenv) so it runs in the e2e CI job.
Phase C added — live layered cold-vs-hot startup benchmark
Measured on this container for Design + methodology: |
The hot arm fed Ready::Executable a fully-resolved absolute path, hitting Executable's on-disk branch, whose bin/ vs exe/ handling diverges (an exe/ path is read directly; a bin/ path collapses to a relative "bin/<name>"). Production `ready gem <exe>` never sees this — it passes the bare name and resolves via Gem.bin_path, reading the real file regardless of bin/exe. Mirror production: render the hot stub source by name in a scrubbed, unbundled subprocess (the bench runs under this project's bundle, where the target gem, e.g. ronin, is not bundled). Drops the exe/-symlink workaround and generalizes the benchmark to any gem by name. Verified: ronin (bin/) and irb (exe/) both render and dispatch; rubocop clean; fast bench suite 10/0.
…t a no-op The cold arm reported `rubygems ~0.2ms` — a hollow number. With a normal boot the interpreter autoloads RubyGems before ruby_up (the RUBYOPT prelude) fires, so the stub's `require "rubygems"` was a $LOADED_FEATURES no-op, and the real ~45ms of RubyGems init sat unlabeled inside the `launch` span. Boot both shims with --disable-gems (accepted in RUBYOPT and propagated through rbenv exec, both verified). Now ruby_up fires before RubyGems loads, the stub's real `require "rubygems"` lands in the `rubygems` span, and `launch` collapses to the true interpreter+shim cost. Total is preserved; rbenv_overhead stays clean because both arms are rubygems-free at launch. Result (ronin): launch 61->10ms, rubygems 0.3->45ms, full ~unchanged. This matches the talk's rubygems-stub layer (and the `ruby --disable=gems -e 'require "rubygems"'` ~45ms measurement).
The prior comment said RubyGems is "autoloaded" at startup. That is wrong: Kernel#autoload is lazy, constant-triggered loading, which is not how RubyGems loads. MRI eagerly performs an implicit `require "rubygems"` during interpreter init (gated by the default `gems` feature), before any RUBYOPT -r option. --disable-gems does not change what runs: it's the same `require "rubygems"` executing the same rubygems.rb (~45ms). It only moves the trigger from the interpreter's implicit startup require (before ruby_up) to the stub's own explicit require (after ruby_up), so the cost lands in a markable span. Reword the comment to say exactly that.
…sion
Ready::CLI never included CommandKit::Options::Version, so --version fell
through to OptionParser's default ("ready: version unknown") and -V was
unregistered ("invalid option"). Include the mixin and set the version from
Ready::VERSION; both flags now print "ready <version>" and exit 0.
… gem `ready up|compile|clobber` shell out to rake against the project Rakefile, whose top requires dev-only tooling (bundler/gem_tasks, rspec, rubocop, gempilot). bundler/gem_tasks runs Bundler::GemHelper.install_tasks at load, which aborts with "Unable to determine name from existing gemspec" whenever the rake cwd lacks exactly one gemspec — guaranteed for an installed gem, since the gemspec is excluded from spec.files while the Rakefile ships. None of that tooling is needed at runtime: the ready/compile/clobber/ready:* tasks live in rakelib/ready.rake, which rake auto-loads independently of this file. Guard the dev-only body on gemspec presence (a source checkout) so an installed gem loads only the runtime tasks. Regression test spawns rake against the Rakefile with no gemspec and asserts the abort no longer occurs.
…e server readyinit set up the ready_* aliases but never exported BY_SOCKET, so the bare `by` command (aliased to ready_by) fell back to the by client's default ~/.by_socket and failed with "No such file or directory - connect(2)". Gem stubs bake BY_SOCKET inline per call, but the interactive `by` relies on the env. Export BY_SOCKET=$READY_SOCK_PATH — the socket the server listens on and the stubs use.
…stub compile_by/compile_gem opened the target with File.open(target, "w"), which truncates before `ready compile` runs. A failed compile then destroyed the last-good stub and left a broken/partial file that rake treats as up to date on later builds, so zsh loads garbage (e.g. a usage string) as the stub body and errors on it. Write to a temp file and rename into place only on success; remove the temp file on failure, leaving the previous stub intact.
…c compile Revert the atomic temp-file-and-rename stub compilation (393b2a1). It added complexity and a new failure surface (stray .tmp files) to make the build corruption-proof, when clobber already is the reliable reset: `ready clobber` removes the build dir so the next build regenerates every stub from scratch, sidestepping the stale-mtime "up to date" trap entirely. Instead, point failures at it: when readyup's compilation task fails, tell the user to run `ready clobber` and rebuild. Loud failure + one reset button beats whack-a-mole hardening.
gillisd
left a comment
gillisd
left a comment
There was a problem hiding this comment.
I had to stop reading because this lacks clarity around all the components, what their roles and behaviors are etc. And more comments is not the solution to this. You need to stop obsessing over primitive types and use real objects instead, that communicate behavior to the reader via proper naming and prose.
| # bench_envelope <run_id> -- <cmd...> | ||
| # Bare-read envelope_start (write deferred until after return so the ~48us log | ||
| # write never lands inside the span), run cmd, read envelope_end first. | ||
| bench_envelope() { |
There was a problem hiding this comment.
Envelope? Why not use the word harness?
There was a problem hiding this comment.
Renamed in 1815be1: the function is bench_harness and the marks are harness_start/harness_end. No "envelope" remains in code, specs, or generated strings.
| export READY_RUN_ID=$rid | ||
| local __start=${EPOCHREALTIME} | ||
| "$@" | ||
| local __end=${EPOCHREALTIME} |
There was a problem hiding this comment.
declare these two locals first, as empties so that there isn't any question that the harness is leaking time
There was a problem hiding this comment.
Done: local run_id= start_time= end_time= declared empty up front; between the two clock reads nothing runs but the command itself, and both log writes are deferred until after the run.
| @@ -0,0 +1,20 @@ | |||
| RSpec.describe Ready::Bench::Aggregator do | |||
| subject(:agg) { described_class.new(span_kind: { "shell" => :min, "tool_run" => :median }) } | |||
There was a problem hiding this comment.
Why are you using strings as keys? I am seeing that throughout this codebase
There was a problem hiding this comment.
Symbols end to end now — span labels and mark names are symbols everywhere. Strings exist only as text in the marks log file and are symbolized at the parse boundary (MarksLog#mark_times_for).
| @@ -0,0 +1,20 @@ | |||
| RSpec.describe Ready::Bench::Aggregator do | |||
| subject(:agg) { described_class.new(span_kind: { "shell" => :min, "tool_run" => :median }) } | |||
There was a problem hiding this comment.
unidiomatic - do not use "agg", abbreviated. write it out
There was a problem hiding this comment.
Moot — Aggregator is deleted entirely (see the #combine thread), so agg went with it. No abbreviated names remain in the bench code.
| subject(:agg) { described_class.new(span_kind: { "shell" => :min, "tool_run" => :median }) } | ||
|
|
||
| it "takes the min for process spans and median for in-process spans" do | ||
| runs = [ |
There was a problem hiding this comment.
why are you calling these spans and then naming the variable runs? Explain to me what you are considering to be a Run vs a Span
There was a problem hiding this comment.
Defined, in code: a Run is one timed invocation of the tool under one arm — cold.3 is round 3's cold run — holding the instant of every mark its layers recorded (run.rb). A Span is a named interval between two of those marks, carrying its own summary statistic (span.rb). The variable you flagged was the tell: combine received per-run span hashes and called them runs because no entity existed to say otherwise. Now MarksLog#run(id) returns a Run, Waterfall.of(run) measures its spans, and Waterfall.summarizing(waterfalls) collapses many runs.
| def real_stub = Pathname(`rbenv which #{@exe}`.strip) | ||
|
|
||
| def instrument! | ||
| FileUtils.mkdir_p(@workdir) |
There was a problem hiding this comment.
Went one further than the include: workdir.mkpath — Pathname already covers it, so the FileUtils dependency is deleted from the class entirely (consistent with the Pathname-everywhere direction). Say the word if you want include FileUtils instead and I will switch it.
|
|
||
| def instrument! | ||
| FileUtils.mkdir_p(@workdir) | ||
| (@workdir / "stub").write(self.class.instrument_stub(real_stub.read)) |
There was a problem hiding this comment.
do not double nest method calls. separate into intermediate var for clarity
There was a problem hiding this comment.
Split: write_instrumented_stub builds the stub in a named intermediate before writing, and RubygemsStub.for_executable resolves stub_path, guards an empty resolution with an explicit error (previously a bare Errno::ENOENT from File.read("")), then reads stub_source — no nested work-doing calls remain.
| end | ||
|
|
||
| # Command word for a run: `<exe>` (rbenv arm) or `<exe>_direct`. | ||
| def command(direct:) = direct ? "#{@exe}_direct" : @exe |
There was a problem hiding this comment.
why aren't you using polymorphism here? this is a textbook use case, and would make this all easier to follow
There was a problem hiding this comment.
It is polymorphic now: abstract Shim owns the script skeleton (shim_start mark, RUBYOPT prelude arming, --disable-gems) as a template method, and RbenvShim/DirectShim supply command_word and exec_line — plus a post_initialize hook so the subclass never calls super. command(direct:) is gone; Runner asks a shim object for its command_word.
|
|
||
| def write_shim(path, rbenv:) | ||
| prelude = Ready.root / "bench" / "prelude.rb" | ||
| target = if rbenv |
There was a problem hiding this comment.
again, major smell/antipattern of something that should be using polymorphism. You pass a primitive that could fall into 2 forms into a method, also pass its type, operate on that primitive, and make decisions about it based on its type. Needs a refactor.
There was a problem hiding this comment.
write_shim(path, rbenv:) is gone with the same refactor: each variant writes itself (shim.write!), so no primitive-plus-type crosses a method boundary anywhere in the arm. The only remaining conditionals select which object to use, never what a value means.
| runs = Hash.new { |h, k| h[k] = {} } | ||
| File.readlines(path).each do |line| | ||
| run, name, t = line.split | ||
| runs[run][name] = t.to_f |
There was a problem hiding this comment.
The nested-hash plumbing is replaced by entities: MarksLog owns the file and symbolizes at the parse boundary, MarksLog#run(id) returns a Run that encapsulates its mark times (recorded?, milliseconds_between), Span#measure(run) asks the run rather than digging, and Waterfall holds the measured spans. Marks and its Hash.new { |h, k| h[k] = {} } are deleted.
vendor/ is bundler's BUNDLE_PATH install target (~4.4k files), .idea/ is JetBrains config, and talk.tar.gz is a throwaway archive of already-tracked files. None belong in git; ignore them so status stays legible.
The harness passed {String => Float} hashes everywhere and called them
different things in different places -- the review's core objection.
Name the domain instead:
- MarksLog/Run/Span/Waterfall/ArmResult replace Marks + Aggregator;
span labels and mark names are symbols, and each Span declares its
own summary statistic (an unknown one now raises)
- Shim + RbenvShim/DirectShim replace the boolean type-switching
(write_shim(path, rbenv:), command(direct:))
- RubygemsStub owns the stub surgery; MarkHelper is the single injected
ready_bench_mark snippet, replacing __bmark, __bp, and the inline
prelude one-liner (bench/prelude.rb is now generated per-workdir)
- bench_envelope -> bench_harness with harness_start/harness_end marks;
locals declared empty before the timed region
- Report owns all rendering including the rbenv overhead line; the pty
cross-check now uses the real pty wall clock (was self-referential);
rbenv: false now actually disables the rbenv probe (was ignored)
- specs rewritten in domain language; PtyShell spec uses subject
Also: dead real_shim/bench_mark deleted, the unused dep_activated mark
dropped, and the vendored references/ excluded from rubocop.
Per review direction, no File.expand_path/File.join: Configuration's sock/readyfile defaults and Executable's require_relative rewrite now join and expand via Pathname.
Set TargetRubyVersion to 3.4 (the gemspec's required_ruby_version, which CI also exercises) so accidental 4.0-only syntax is caught instead of shipped, and the Gemspec/RequiredRubyVersion warning clears. Because of this the `require "pathname"` at the entry point is no longer flagged as redundant -- it is genuinely needed on 3.4, where Pathname is not autoloaded, so a bare production install would NameError without it. Exclude extra/ (standalone runtime helper scripts) as already done for references/. Alongside the mechanical autocorrections: remove the dead `render_environment_string` (no caller; the template inlines env), fix a latent `raise ArgumentError "..."` missing its comma in fetch_env's untested branch, and extract `resolve_fallback`/`validate_*!` to satisfy the metrics cops without changing behavior.
Convert message-expectation mocks to spies (allow + have_received), add :aggregate_failures where examples hold several checks, and replace trivial `match(/.../)` assertions with exact `eq`/`include`. rake_command_spec stops stubbing the object under test (RSpec/SubjectStub): it now drives a real `rake` shell-out against a temp Rakefile and asserts interpreter, cwd, and exit-status propagation for real. readyfile_spec drops its @ivar for a build_dir helper. Every assertion is preserved or strengthened.
The sandbox runs `ready up` inside Bundler.with_unbundled_env (simulating a real user's non-bundler install), which strips the bundle's bin path. With bundler-cache's path install, by-server lives only there, so the daemon spawn hit Errno::ENOENT. Install the pinned by (1.1.0) as a system gem -- the same place rbenv puts it locally and on macos-e2e -- so it survives the unbundled env.
benchmark_spec runs the Runner with `rbenv: false` (the non-rbenv path the ubuntu e2e job takes), but the flag was ignored below the Runner: RubygemsStub.for_executable always shelled out to `rbenv which`, so the cold arm died with Errno::ENOENT on any host without rbenv. Thread `rbenv:` through ColdArm into RubygemsStub. Without rbenv, resolve the same rubygems stub as `<ruby bindir>/<name>` -- which is exactly what `rbenv which` returns when rbenv is present (verified identical for `ri`). Also skip writing the rbenv shim when rbenv is off; it only exists to measure the `rbenv exec` overhead the runner already probes only when `@rbenv`.
The benchmark's render_production_source runs `require "ready"` in a scrubbed, unbundled subprocess (to resolve the target gem like a real install would), and that needs zeitwerk -- ready's only non-default runtime require (verified: the scrubbed process activates only zeitwerk plus default gems). With bundler-cache it lives only in the bundle, so pin-install it system-wide alongside by, both at their locked versions.
The waterfall table is exhaustive but not graspable at a glance. Add a six-row summary under the headline -- shell, rbenv shim, rubygems (interpreter boot folded in), activate deps, the tool, total -- each backed by the cold arm's real measured span(s), for dropping onto a slide.
The slide summary mixed statistics -- layer rows used each span's median while the total used the minimum floor (matching the headline). For a light dependency graph the two nearly agreed, but a heavy one (ronin: activation ~350 ms) made the median layers overshoot the min-based total, so the parts summed to more than the whole -- which reads as broken on a slide. Use each layer's minimum floor, the same statistic the total uses, so the layers always sum to at most the total.
The harness timed every run between its envelope marks no matter whether the tool succeeded, and sent tool output to /dev/null -- so a tool that crashed silently was recorded as a fast "success". Two fixes: - Every measured command tees stdout+stderr to ./log/bench.log (HarnessLog) so a failure leaves evidence instead of vanishing. - bench_harness records each run's exit status; a run that exits non-zero (or records none) aborts the whole benchmark, naming the run and pointing at the log, instead of producing a phantom measurement. Verified end-to-end: ri TCPServer completes (16x), rake nosuchtaskxyz aborts on hot.1 with "exited 1"; by propagates non-zero worker exits so the hot arm is covered. Guarded against zsh's read-only `status` special (exit code lands in `exit_code`).
When the persistent by-server has test/unit loaded, its at_exit auto-runner
parses the process ARGV -- so any option a ready'd CLI leaves in ARGV is
rejected ("invalid option: --format=ansi", printed as a Test::Unit usage
banner for by-server). A dispatched CLI is never a test run, so the rendered
stub now sets Test::Unit::AutoRunner.need_auto_run = false before the CLI's
source.
Reproduced: by-server with test/unit loaded + an option the CLI leaves in
ARGV -> the Test::Unit 'invalid option' banner. With the prologue line the
same dispatch runs clean.
This reverts c3f5e66. Injecting a test/unit-specific conditional into every rendered CLI's core source is duct tape in the wrong layer -- the renderer shouldn't know about test frameworks. The real cause is test/unit being loaded into the persistent server; the fix is to not preload it (readyfile), not to defensively disable its auto-runner in each stub.
The slide summary folded the VM boot (launch) into the rubygems row. That makes 'ready slashes rubygems' untruthful: both arms boot a Ruby VM (ready's by client boots one too), so lumping the boot into rubygems implies ready eliminates the boot, which it does not. Give the boot its own 'ruby vm boot' line; 'rubygems' is now just the framework load -- the cold-only part ready actually eliminates.
Split the rbenv-shim example into two (the added 'ruby vm boot' label pushed the layer-order assertion over RSpec/ExampleLength), and exclude the gitignored log/ scratch dir (run logs + throwaway report-parsing scripts) from rubocop.
results.md: cold-vs-warm startup across 8 verified non-interactive Ruby CLIs, with the ruby-vm-boot layer split out from rubygems, the rbenv-shim noise kept separate, and the honest exclusions (rbs/rougify via __FILE__-under-eval, rspec via the rspec-core resolver gap). readyfile: a worked ronin example whose gems: entries are require paths (ronin, ronin/support), not gem names.
Redo the CLI benchmark with realistic workloads instead of --version: ri (ansi doc render), colorls -l, ronin encode, youplot line, kamal accessory tree, codeball pack. Every run is verified (exit-check plus a hot-arm output cross-check), and the report splits ruby-vm-boot out from rubygems. run_multibenchmark_test.sh reproduces it push-button (installs gems incl. codeball's libmagic, builds shared fixtures, runs all six).
Bare 'ready compile' errored (insufficient arguments). Make the names argument optional and treat an empty invocation the same as 'all', so 'ready compile' compiles every stub -- the way bare 'make' runs its default target. Usage/examples/docs updated to show the default.
Rubyconf final
1 participant
Gives
readythe one test class it structurally lacked: proof that a generated zsh stub, loaded into a live interactive shell, aliases the bare command and dispatches through the persistentby-server— plus the pure-Ruby engine for a cold-vs-hot startup waterfall.Stacked on
cli-polish(#1); diff is the 9 e2e commits only.Phase A — functional E2E harness (
:e2e, opt-in)Ready::PtyShell— drives a realzsh -f -iunder a pty; printed-marker sync (DO''NEquote-split) so command echo can't be mistaken for output; raises loudly on desync/timeout.Ready::Sandbox— builds an isolated runtime in a temp prefix (ready up: compile stubs + bootby-server), exposes the env a pty needs to attach, and tears it all down (runsby-server stopwhile the socket is present — the path the rake task botches — then reaps by argv, then removes the tree). Drops a staleGEM_HOME/GEM_PATHleft by a removed version manager so the build resolves from the project bundle.spec/e2e/dispatch_spec.rb— sources the plugin against a live socket (readyinitfast path), assertswhence -v rake→rake is an alias for ready_rake, and thatrake --versiondispatches through the by-server and returns the real version.:e2eexcluded from the fast loop unlessREADY_E2Eis set;rake spec:e2etask; a dedicated zsh-enablede2eCI job (both Rubies).rake spec:e2e→ 4 examples, 0 failures; zero orphaned servers.Phase B — waterfall analysis library (fast suite)
Ready::Bench::Marks/Stats/Report— parse a marks log into a per-span millisecond cold-vs-hot waterfall with paired deltas and a pty cross-check. Unit-tested over fixture logs (no runtime deps).rake spec→ 33 examples, 0 failures.Zeitwerk
The harness is autoloaded via a second
Zeitwerk::Loaderforspec/support(namespaceReady) — no manualrequireglob.rake zeitwerk:validatestill passes.Follow-up (not in this PR)
Phase C — the live cold-vs-hot benchmark that produces real marks to feed
Ready::Bench::Report— needs env-gated mark instrumentation in the gem runtime and is scoped as a separate, brainstorm-first change.🤖 Generated with Claude Code