Make the spec suite runnable and trustworthy again - #8
Merged
IgorFroehner merged 9 commits intoAug 13, 2026
Merged
Conversation
The suite has not been able to boot for some time. Three separate things had to line up: concurrent-ruby >= 1.3.5 dropped its implicit `require 'logger'`, which Rails < 7.1 relies on, so every spec file died at load with `NameError: uninitialized constant ActiveSupport::LoggerThreadSafeLevel::Logger`. rubocop could not start either, for an unrelated dependency conflict in the stack jetrockets-standard pulls in. Both are pinned in the Gemfile rather than the gemspec, since they are development constraints and should not be inflicted on consumers. Both come out with the Rails upgrade. database.yml was gitignored, so a fresh clone had nothing to connect with. It is committed now and reads every value from the environment, so local setups and CI can point elsewhere without editing it. Ruby is pinned to 3.3.11: it is the newest version Rails 6.1 can run on, since 3.4 drops mutex_m from the default gems, and unlike 3.1 it is not already end of life. bin/setup now creates the database instead of only running bundler.
The dummy app only ever had a securities table, so Recorder::Revision had no table behind it and nothing could exercise the gem end to end. The revisions migration mirrors the generator template so that the specs test what users actually install; a comment on each side asks for them to be kept in sync. The one intentional difference is the versioned `ActiveRecord::Migration[6.1]` — the template still subclasses the bare class, which has been unsupported since Rails 5, and fixing that belongs in its own change. users exists because Recorder::Revision belongs_to :user and the gem expects host apps to supply that constant. schema.rb is regenerated by db:migrate rather than edited by hand, which reformats it, so it is excluded from rubocop.
Security included Recorder::Observer but never called `recorder`, and including the module alone only adds the `revisions` association — it is the macro that registers the after_create/update/destroy callbacks. So no revision was ever recorded in the dummy app, whatever the specs did.
Recorder keeps state in three places that all outlived a single example, and all three leaked: rows stayed in the database, Config is a Singleton, and Store memoises a RequestStore that is never cleared outside a real request cycle. Rows survived across whole rspec processes too. The store leak is the dangerous one. A stray `recorder_disabled!` carries into later examples, which then record nothing and pass anyway — the exact failure mode that lets a broken observer look healthy. RailsExampleGroup is included explicitly rather than leaning on rspec-rails' global FixtureSupport include, which is deprecated (rspec/rspec-rails#1355) and due to be removed in rspec-rails 7. Without one of the two, plain RSpec.describe groups get no transaction at all, since nothing here lives in a Rails-typed directory to infer it from. isolation_spec guards the lot. Both examples assert a clean slate and then dirty it, so whichever runs second catches a leak and the guard holds under `config.order = :random` without pinning an order.
security_spec.rb was a require line with no examples. Everything the suite covered until now stubbed the pieces in isolation, so nothing proved that including Recorder::Observer records anything at all. Covers create, update, a no-op update, destroy, and the disabled path, against real callbacks and real rows. Deliberately a smoke test rather than exhaustive observer coverage.
The Rakefile required rails/dummy/tasks, which registers `rake dummy:app`.
Its first step is FileUtils.rm_rf('spec/dummy') before regenerating the
app from scratch. This dummy app is hand-maintained and checked in — it
has models, migrations and a database.yml the specs depend on — so
running that task destroyed the fixtures.
Nothing else used the gem.
master.key was checked in alongside credentials.yml.enc, which defeats the point of encrypting them. Nothing in the dummy app reads credentials.
Several comments narrated what the code replaced or justified choices by contrast with a previous arrangement, which belongs in commit messages rather than in the files. Dropped those, and shortened the rest to what a reader needs at the point of use.
It asserted the behaviour of spec/rails_helper.rb rather than of the gem, which is not something worth carrying in the suite.
IgorFroehner
force-pushed
the
chore/make-suite-runnable
branch
from
August 12, 2026 15:29
fbe78f6 to
553ab52
Compare
IgorFroehner
marked this pull request as ready for review
August 12, 2026 17:35
Member
|
@IgorFroehner before I will be able to merge this, we need to do one very important thing – automate tests running in CI. Please do this in a separate PR. |
Collaborator
Author
@igor-alexandrov the idea is to merge this and have the tests working and passing, and then create the CI pull request with the CI green. As you can see in this PR we have the CI green #9. |
This was referenced Aug 13, 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
2 participants
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.
First maintenance PR after a long gap. This one does no behavior changes to the gem, it just
makes the test suite run again, and makes it tell the truth when it does.
Why
bundle exec rspecloaded zero examples on any modern Ruby, andbundle exec rubocopcould not start at all. Both were dependency bit-rot, notanything in our code. On top of that, the dummy app had no
recorder_revisionstable and
Securitynever actually calledrecorder, so nothing had everexercised the gem end to end — and the suite had no isolation, so any spec that
did would have been unreliable.
What's here
Seven commits, each one green on its own so the history stays bisectable:
recorder_revisionsanduserstables to the dummy appSecuritymodelSecurityrevisionsrails-dummyand its destructivedummy:apptaskGetting it running
concurrent-ruby >= 1.3.5dropped its implicitrequire 'logger', whichRails < 7.1 relies on — every spec file died at load. Pinned, along with a
second pin for an unrelated conflict that stopped RuboCop starting. Both are
in the
Gemfile, not the gemspec: they are development constraints andshould not reach consumers. Both come out with the Rails upgrade.
database.ymlwas gitignored, so a fresh clone had nothing to connect with.It is committed now and reads every value from
ENV.mutex_mfrom default gems), and unlike 3.1 it is not already EOL.bin/setupcreates the database instead of only running Bundler.Making it honest
State leaked three ways, all verified: rows survived not just examples but
entire
rspecprocesses,Recorder::Configis a Singleton, andRecorder::Storememoises aRequestStorethat is never cleared outside a realrequest cycle.
The store leak is the one that matters. A stray
recorder_disabled!carriesinto later examples, which then record nothing and pass anyway — precisely
the failure mode that lets a broken observer look healthy.
spec/rails_helper.rbnow rolls back the database, resets the
Configsingleton and clears theRequestStorebefore every example.Two things found along the way
Securitynever calledrecorder. IncludingRecorder::Observeraloneonly adds the
revisionsassociation; the macro is what registers thecallbacks. Nothing was ever recorded in the dummy app.
rake dummy:appdeleted the fixtures.rails-dummyregisters it, and itsfirst step is
FileUtils.rm_rf('spec/dummy')before regenerating fromscratch — but our dummy app is hand-maintained and checked in. Gem removed.
Also dropped
master.key, which was committed next tocredentials.yml.enc.Verification
From a clean checkout with the database dropped:
./bin/setup && bundle exec rake→ 40 examples, 0 failures. Repeated runs stay green with zero leftover rows.
RuboCop now runs and reports the same 5 pre-existing offences as before — this
PR adds none.