RSpec testing in Bike Index

This project uses RSpec. All business logic should be tested.

Run only the specs your change touches

AGENTS.md has the rule. When something fails outside the files you changed, re-run that spec file on its own first: failing alone means it's real and gets fixed, never excused as pre-existing; passing alone is the fixing-flaky-failures skill, whose first rule is that you can't reach for a retry, a looser matcher, or a deleted assertion to make it green.

What a spec prints doesn't reach you

rtk's rspec wrapper reports a summary in place of the run's output, which costs you two different things. A puts added to a scratch spec to inspect a value vanishes and the run reads as an ordinary pass — redirect it (bundle exec rspec spec/foo_spec.rb > tmp/probe.log 2>&1) or write to a file from inside the example, as fixing-flaky-failures does for browser-side values. A failure keeps its ExpectationNotMetError: line but loses the expected/got values under it, so re-running the one example tells you no more than the first run did; rtk proxy bundle exec rspec <file>:<line> is what shows them. --format documentation rescues neither.

What to test (and what not to)

• Tests should either: help make the code correct now, or prevent bugs in the future. Don't add tests that don't do one of those things.
• Use request specs, not controller specs — request specs go through the full middleware/routing stack, so they catch breakage controller specs can't see. Everything making the same request should be in a single test. For markup a component owns, see A component's own markup.
• Avoid testing private methods.
• Avoid mocking objects.
  • If making external requests, use VCR. Never write or edit a cassette by hand — record them by running the tests (see VCR cassettes).
• Don't use tap to bundle factory creation with follow-up setup. Create the record in let/let!, then do the follow-up work on its own line (a separate statement, or a before block). One thing per line reads better and keeps the factory call clean.

Good

let!(:bike_transferred) { FactoryBot.create(:bike, :with_ownership_claimed, user:) }
before { BikeServices::OwnershipTransferer.find_or_create(bike_transferred, updator: user, new_owner_email: "new@example.com") }

Bad

let!(:bike_transferred) do
  FactoryBot.create(:bike, :with_ownership_claimed, user:).tap do |bike|
    BikeServices::OwnershipTransferer.find_or_create(bike, updator: user, new_owner_email: "new@example.com")
  end
end

Session state in a request spec comes from a real request

A request spec reads session and assigns freely, but a write doesn't outlive the call — the next request rebuilds the session from the cookie. There is no controller spec to fall back on; spec/controllers/ is gone.

So drive the request that sets the key: /oauth/authorize?partner=…&company=… for partner/company (arrive_via_partner in :existing_doorkeeper_app), /session/new?return_to=… for return_to, a bike's recovery link for recovery_link_token, a page render as an organization member for passive_organization_id. Where a param reaches the same code — return_to is read from params too — prefer the param over a priming request. spec/requests/sessions_request_spec.rb and spec/requests/users_request_spec.rb are the patterns.

A component's own markup is tested in its component spec

What a component decides about its markup — a label, a placeholder, a class, whether a field renders at all — belongs in spec/components/**/component_spec.rb, not in the request spec for a page that happens to render it. Request specs cover the request: status, redirects, what was saved, what the page is wired to.

A component having no spec yet isn't a reason to put it in the request spec instead. spec/components/pages/register/views/step2/component_spec.rb is the pattern, including the render_x helper that reloads the record so an object updated mid-example isn't answered from the copy the previous render left behind.

No stylesheet loads here, so Capybara calls anything hidden by a class visible. have_no_field/visible: false against a tw:hidden wrapper fails with "found 1 match", which reads as the component rendering the wrong thing. Assert the wrapper instead — page.find("form div.tw\\:hidden input[name='additional']", visible: :all) — and leave real invisibility to a :js system spec.

render_in_view_context takes its subjects as method arguments

A component needing a form_builder renders inside render_in_view_context { form_for … }, which instance_execs its block in the view context — so let values aren't in scope and a bare organization raises NameError. Wrap it in a def rendered_component(organization) and call that from the let(:component); the block closes over the method's locals. spec/components/pages/admin/organizations/form/wrapper/component_spec.rb is the pattern.

display_dev_info? is false in test, so nothing it gates is verifiable

ControllerHelpers#display_dev_info? opens with !Rails.env.test?. Every only-dev-visible block it wraps is unrendered in the suite, so threading the flag through a component — a wrong default, a missed hop — passes green and is wrong only in development. Reading the call sites isn't enough either — it can't show that a UI::Table cell block is instance_exec'd, so an @ivar in one resolves against the table and is always nil. Check it in the browser signed in as dev@bikeindex.org; the flag needs developer? and MiniProfiler, so the superadmin banner button won't do it.

log_in stubs the auth lookup, so it can't answer whether a session ends — or whether a user is confirmed

spec/support/request_spec_helpers.rb's log_in (and every :request_spec_logged_in_as_* context) stubs User.from_auth to return the user, so the cookie is never read and the session outlives anything done to that user — deleting, banning, rotating their auth_token.

The stub also answers User.unconfirmed.from_auth, so unconfirmed_current_user is always present. Sessionable#skip_if_signed_in asks that before it asks whether the user is confirmed, so every request it guards — /session/new, /session/magic_link, /users/new — redirects to please_confirm_email under log_in, whatever the user is.

Either one means signing in for real: include_context :request_spec_signed_in_for_real, which posts real credentials and takes a let(:user) override. spec/requests/sessions_request_spec.rb's "deleted after signing in" and its describe "destroy" are the patterns.

VCR cassettes: never hand-edit, always re-record

Never open a cassette and change it. Not a URL, not a token, not a timestamp, not an interaction — no matter how small or how obviously right the edit looks. A cassette is a recording of what a real service actually said; an edited one asserts something no service ever returned, and the spec passes against a fiction. This has no exceptions.

The only way a cassette changes is a spec run that records it:

• Stale or wrong contents — rm the file and run the spec. VCR writes it from scratch, holding exactly the requests the spec makes now. Re-recording without deleting only writes back the requests still being made, so interactions the spec has stopped making survive forever — and because VCR times re_record_interval from the cassette's oldest interaction, one stale entry re-triggers re-recording on every run.
• Cassette missing for a resource that doesn't exist yet — leave it absent and the spec red. Don't fabricate one.
• Secrets — spec/rails_helper.rb's filter_sensitive_data and before_record handle them. Add the key there; don't scrub the file by hand.

Commit what a run re-records. A modified cassette is a re-recording, not unrelated churn — cassettes carry a re_record_interval and are meant to update. Commit it on the branch you're on, whatever that branch is about. Never git checkout it away to keep a diff focused.

git status after a spec run is the only signal; a run that re-records prints nothing.

Never write WebMock.stub_request. HTTP in a spec is a cassette, with no exceptions. A stub asserts what you imagined a service returns; a cassette records what it actually returned, which is the same reason cassettes are never hand-edited. The handful of WebMock.stub_request calls still in spec/ are legacy — existing usage is not a precedent to copy.

Stubbing ENV

Never partial-mock ENV with allow(ENV).to receive(:[]).and_call_original — it makes every subsequent ENV[...] lookup go through RSpec's message router, which is slow and easy to break by forgetting a .with(...) branch.

Use stub_const against a merged hash instead:

Good

stub_const("ENV", ENV.to_hash.merge("STRIPE_SECRET_KEY" => "sk_test_123"))

Bad

allow(ENV).to receive(:[]).and_call_original
allow(ENV).to receive(:[]).with("STRIPE_SECRET_KEY").and_return("sk_test_123")

Drain Sidekiq jobs, don't run them inline

Run enqueued jobs by draining them in the default fake mode — SomeJob.drain for one job, Sidekiq::Job.drain_all for everything (clear first with Sidekiq::Job.clear_all when earlier setup left jobs queued). Don't wrap the exercise in Sidekiq::Testing.inline!. Draining lets the request finish and commit before the jobs run, against that committed state — the way production does it — and keeps the test from silently pulling in every cascading job.

Always fix failing tests

Fix every failing test, even ones that were already failing on main. Confirming a failure pre-dates your branch (via git stash or checking out main) explains what broke — not whether you fix it. You fix it.

Don't weaken assertions to make a failing test pass

Reproduce the failure and find out what changed before touching the expectation. Changing an expected value to whatever now renders, loosening eq to include, dropping a count:, or deleting the assertion with a "looks unrelated" handwave all erase signal rather than fix anything. Fix the code if the original assertion was right; update it — with a comment — if the behaviour intentionally changed. The fixing-flaky-failures skill has the same rule for the intermittent case, where it is absolute.

Match a target attributes hash, not one attribute at a time

When you're checking several fields on the same object or response, build one expected-attributes hash and assert against it in a single matcher. Don't write a chain of one-attribute-per-line expects.

• Object (ActiveRecord, plain Ruby): expect(record).to have_attributes(target_attributes)
• Hash (JSON response, parsed body): expect(hash).to eq(target.as_json) for full match, or expect(hash).to include(target_attributes) for partial.

This collapses what would be 4 brittle assertions into 1, makes the contract visible at a glance, and gives a single readable diff when something changes. It also avoids the trap of weak per-field assertions like expect(x).to be_present or expect(url).not_to include("blank.png") standing in for "the right value" — match the value directly.

Good

target_attributes = {kind: "found", impounded_description: "Some description"}
expect(impound_record).to have_attributes(target_attributes)

expect(json_result["memberships"]).to eq([target_membership.as_json])

Bad

expect(impound_record.kind).to eq("found")
expect(impound_record.impounded_description).to be_present
expect(impound_record.impounded_description).to eq("Some description")

logo_url = json_result["memberships"].first["organization_logo_url"]
expect(logo_url).to be_present
expect(logo_url).not_to include("blank.png")
expect(logo_url).to eq(organization.avatar_url)

Structuring with context and let

Use context and let to isolate what varies between examples. Each it block should live in a context that names the condition, with let overrides for only what differs in that case. Avoid repeating setup across sibling it blocks.

Good

describe "show_bulk_import?" do
  let(:organization) { FactoryBot.build(:organization, pos_kind:) }
  let(:pos_kind) { "no_pos" }

  it "is falsey" do
    expect(organization.show_bulk_import?).to be_falsey
  end

  context "when ascend" do
    let(:pos_kind) { "ascend_pos" }

    it "is truthy" do
      expect(organization.show_bulk_import?).to be_truthy
    end
  end

  context "when broken_ascend_pos" do
    let(:pos_kind) { "broken_ascend_pos" }
    it "is truthy" do
      expect(organization.show_bulk_import?).to be_truthy
    end
  end

  context "when lightspeed_pos" do
    let(:pos_kind) { "lightspeed_pos" }
    it "is truthy" do
      expect(organization.show_bulk_import?).to be_falsey
    end
  end

  context "when feature show_bulk_import_impound" do
    let(:organization) { FactoryBot.build(:organization_with_organization_features, enabled_feature_slugs: ["show_bulk_import_impound"]) }
    it "is truthy" do
      expect(organization.show_bulk_import?).to be_falsey
    end
  end
end

Bad

it "returns truthy for show_bulk_import?" do
  organization = FactoryBot.create(:organization, pos_kind: "ascend_pos")
  expect(organization.show_bulk_import?).to be_truthy
end
it "returns truthy when feature is included" do
  organization = FactoryBot.create(:organization)
  allow(organization).to receive(:any_enabled?) { true }
  expect(organization.show_bulk_import?).to be_truthy
end

The bad version repeats setup, mocks the object, and doesn't communicate what each case represents.

One example per distinct setup — combine same-setup it blocks

context/let/before isolate what varies. The corollary runs the other way: if two sibling it blocks share the same setup — no differing context, before, or let override between them — collapse them into one example. Each distinct setup earns exactly one it; put all of that setup's assertions (and all of its requests/renders) in that single block.

This is the same instinct as "everything making the same request should be in a single test", generalized: splitting same-setup assertions across sibling it blocks re-runs identical setup (factories, HTTP requests, renders) once per block for zero isolation benefit, and scatters one logical behavior across the file. Two it blocks that differ only in the request params or the assertion — with identical lets and no before between them — are one example.

After writing a spec, scan each context/describe: if it holds multiple it blocks and they don't each sit behind a distinct context/before/let, merge them.

Not when the first request changes what the next one does. Same setup isn't the same starting state once a request has run: signing in consumes session[:return_to], and session[:discourse_redirect] is set with ||= so a second arrival can't replace the first. Both merges pass review and go red. Leave those as sibling contexts with a let for what differs, and say in a comment why they can't share one example — otherwise the next cleanup pass merges them again.

A helper that reads the response has to be a def, not a let. One example making several requests is exactly where a let that parses response.body bites: it memoizes the first response and every later assertion re-reads it, so the example fails while the code is right (or worse, passes while the code is wrong). def re-evaluates.

Good

context "superuser" do
  let(:current_user) { FactoryBot.create(:superuser) }

  it "offers every view and renders the owner and org-limited perspectives" do
    get "#{base_url}/#{bike.id}"
    expect(body_text).to match("View as owner of bike")

    get "#{base_url}/#{bike.id}", params: {view_as: "owner"}
    expect(body_text).to match("Your bike")

    get "#{base_url}/#{bike.id}", params: {view_as: "#{org.to_param}.limited"}
    expect(body_text).to match("Limited")
  end
end

Bad

context "superuser" do
  let(:current_user) { FactoryBot.create(:superuser) }   # re-created for every it below

  it "offers every view" do
    get "#{base_url}/#{bike.id}"
    expect(body_text).to match("View as owner of bike")
  end
  it "renders the owner view" do
    get "#{base_url}/#{bike.id}", params: {view_as: "owner"}
    expect(body_text).to match("Your bike")
  end
  it "renders an org panel as limited" do
    get "#{base_url}/#{bike.id}", params: {view_as: "#{org.to_param}.limited"}
    expect(body_text).to match("Limited")
  end
end