credit_spec tests for #credit_count are ineffective and do not fail when expected to

[codeandclay]

Bug: credit_spec tests for #credit_count are ineffective and do not fail when expected to

Credit uses counter culture to create and populate columns. The code currently populates these columns explicitly. Unpopulated credits_count columns should cause tests to fail.

The problem

The column_count tests pass regardless of whether the column is populated or not.

To Reproduce

Comment out the third item in column_names:. (The item that tells counter_culture how to populate credits_count.)

  # app/models/credit.rb

  %i[user organization].each do |type|
    counter_culture type,
                    column_name: ->(model) { "#{model.spent ? 'spent' : 'unspent'}_credits_count" },
                    column_names: {
                      ["credits.spent = ?", false] => "spent_credits_count",
                      ["credits.spent = ?", true] => "unspent_credits_count",
                      # ["credits.id > ?", 0] => "credits_count"
                    }
  end

Now the credits_count columns of user and organization should be empty. Despite this change, the test passes:

➜  forem git:(main) ✗ dip bundle exec rspec spec/models/credit_spec.rb
[Zonebie] Setting timezone: ZONEBIE_TZ="Lima"
...............

Finished in 1.16 seconds (files took 3.65 seconds to load)
15 examples, 0 failures

Remedy

I'm not familiar with RSpec but I believe the problem is because let(:user) { create(:user) } creates the user before it has chance to create the credits: let!(:user_credits) { create_list(:credit, 2, user: user) }. Therefore, a count of zero credits is compared to the size of an empty array. The same goes for organization.

To fix this, I've used the eager loading version let! to ensure the credits are created with the correct associations.

 # credit_spec.rb

  context "when caching counters" do
    let!(:user_credits) { create_list(:credit, 2, user: user) }
    let!(:org_credits) { create_list(:credit, 1, organization: organization) }

The tests now fail.

Failures:

  1) Credit when caching counters #credits_count counts credits for user
     Failure/Error: expect(user.reload.credits_count).to eq(user.credits.size)
     
       expected: 2
            got: 0

  ...

  2) Credit when caching counters #credits_count counts credits for organization
     Failure/Error: expect(organization.reload.credits_count).to eq(organization.credits.size)
     
       expected: 1
            got: 0

After re-instating the commented out line, the tests pass as expected.

# app/models/credit.rb

%i[user organization].each do |type|
  counter_culture type,
                  column_name: ->(model) { "#{model.spent ? 'spent' : 'unspent'}_credits_count" },
                  column_names: {
                    ["credits.spent = ?", false] => "spent_credits_count",
                    ["credits.spent = ?", true] => "unspent_credits_count",
                    ["credits.id > ?", 0] => "credits_count"
                  }
end

➜  forem git:(main) ✗ dip bundle exec rspec spec/models/credit_spec.rb
[Zonebie] Setting timezone: ZONEBIE_TZ="Astana"
...............

Finished in 1.25 seconds (files took 3.58 seconds to load)
15 examples, 0 failures

Addendum: Are spent_credits_count and unspent_credits_count tests satisfactory?

Now, repeating the above for unspent_credits_count and spent_credits_count, I find the test pass regardless.

How do I know if this is expected or not?

I break Credit by swapping the scopes.

# app/models/credit.rb

scope :spent, -> { where(spent: false) } # should be true
scope :unspent, -> { where(spent: true) } # should be false

%i[user organization].each do |type|
    counter_culture type,
                    column_name: ->(model) { "#{model.spent ? 'spent' : 'unspent'}_credits_count" },
                    column_names: {
                      # ["credits.spent = ?", true] => "spent_credits_count",
                      # ["credits.spent = ?", false] => "unspent_credits_count",
                      ["credits.id > ?", 0] => "credits_count"
                    }
end

Tests fail as expected. Failing tests:

Credit when caching counters #unspent_credits_count counts the number of unspent credits for a user
Credit when caching counters #unspent_credits_count counts the number of unspent credits for an organization
Credit when caching counters #spent_credits_count counts the number of spent credits for a user
Credit#add_to adds the credits to the user
Credit#add_to adds the credits to the organization
Credit#remove_from adds the credits to the user

I believe the existing tests are enough to ensure that spent_credits_count and unspent_credits_count are populated correctly and that there is no need to populate these columns explicitly. (Here the counter culture docs give an example of creating columns without the :column_names argument.)

  %i[user organization].each do |type|
    counter_culture type,
                    column_name: ->(model) { "#{model.spent ? 'spent' : 'unspent'}_credits_count" },
                    column_names: {
                      ["credits.id > ?", 0] => "credits_count"
                    }
  end

To be doubly sure, jumping into the tests with pry, I can see that everything seems to be as expected and the tests should pass.

 # credit_spec.rb

describe "#spent_credits_count" do
  it "counts the number of spent credits for a user" do
    create_list(:credit, 1, user: user, spent: true)

    binding.pry

    expect(user.reload.spent_credits_count).to eq(user.credits.spent.size)
  end

[1] pry(#<RSpec::ExampleGroups::Credit::WhenCachingCounters::UnspentCreditsCount>)> organization.reload.unspent_credits_count
=> 1
[2] pry(#<RSpec::ExampleGroups::Credit::WhenCachingCounters::UnspentCreditsCount>)> organization.credits.unspent.size
=> 1
[3] pry(#<RSpec::ExampleGroups::Credit::WhenCachingCounters::UnspentCreditsCount>)> user.reload.unspent_credits_count
=> 2
[4] pry(#<RSpec::ExampleGroups::Credit::WhenCachingCounters::UnspentCreditsCount>)> user.credits.unspent.size
=> 2

I will raise this refactor in discussions.

[github-actions]

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

To our amazing contributors: issues labeled bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

If this is a feature request from an external contributor (not core team at Forem), please close the issue and re-post via GitHub Discussions.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem-team. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

[codeandclay]

NB, I've started a discussion (#20671) asking for guidance on how to demonstrate that the tests are currently broken.