Best Practice Proposal: Don't rely on factory defaults

[harrylewis]

Hey 👋🏻

I really appreciate the work that's been done here to develop these guidelines, and the philosophy surrounding it. It has been a source of inspiration for my team (and spirited debate), helping us push towards our own internal standards.

I'd like to suggest a new best practice. Consider the following example.

RSpec.describe Invoice do
  describe '#draft?' do
    context 'when the status is draft' do
      it 'returns true' do
        invoice = create(:invoice) # <-- Problem: Relying on factory defaults for test setup.

        expect(invoice.draft?).to eq(true)
      end
    end


    context 'when the status is paid' do
      it 'returns false' do
        invoice = create(:invoice, status: :paid)

        expect(invoice.draft?).to eq(false)
      end
    end
  end
end

I've seen many tests like this, where a particular context relies on the default values of a factory for test setup. In my opinion, this violates the philosophy of Even Better Specs.

The test is not self-contained. The test setup relies on a default value set elsewhere (in the factory). If that default changes, this test will break, making the test brittle. This test does not follow the Arrange-Act-Assert pattern explicitly. The test setup that the context depends on happens implicitly using a default value. This can reduce the readability of the code, as I might be surprised to see a lack of test setup corresponding with the test context.

Instead, in keeping with the philosophy, I would suggest explicitly setting up all test data necessary to validate a particular context's setup.

context 'when the status is draft' do
  it 'returns true' do
    invoice = create(:invoice, status: :draft)

    expect(invoice.draft?).to eq(true)
  end
end

I am calling this rule don't rely on factory defaults. I look forward to hearing what you think. I'm happy to draft a pull request if this feels worthy to include.

[glaucocustodio]

hey Harry, I think it makes sense such a guideline 👍🏻, so I'd be more than happy to accept a PR.

Perhaps we should highlight that it's ok relying on factory defaults for things that are not supposed to change with the code under test.

It would be ok as well to rely on factory traits, so one could have:

  it 'returns true' do
    invoice = create(:invoice, :draft)

    expect(invoice.draft?).to eq(true)
  end