Fix intermittent test failures

[vsppedro]

Hi, this pull request is a draft for now.

I'm looking for a solution to the issue #1262.

Below are some notes about the problem.

How to recreate the error locally?

The smallest possible scenario for this error to happen:

BUNDLE_GEMFILE=/path/to/shoulda-matchers/gemfiles/rails_X_X.gemfile bundle exec rspec './spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb[1:3:2:8:1:1:1,1:5:2:2:1:2:1]' --seed 7438

Shoulda::Matchers::ActiveRecord::ValidateUniquenessOfMatcher
  when the model has a case-insensitive validation
    when case_insensitive is specified
      it supports ignoring_interference_by_writer
        when the writer method for the attribute changes incoming values
          and the value change causes a test failure
            lists how the value got changed in the failure message
  when the model has a scoped uniqueness validation
    when one of the scoped attributes is a boolean column
      when there is more than one validation on the same attribute with different scopes
        when a record exists beforehand, where all scopes are set
          when each validation has a different message
            accepts (FAILED - 1)

Finished in X seconds (files took X seconds to load)
2 examples, 1 failure

If we try to run only the test that failed:

BUNDLE_GEMFILE=/path/to/shoulda-matchers/gemfiles/rails_X_X.gemfile bundle exec rspec './spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb[1:3:2:8:1:1:1]' --seed 7438

It pass:

Shoulda::Matchers::ActiveRecord::ValidateUniquenessOfMatcher
  when the model has a scoped uniqueness validation
    when one of the scoped attributes is a boolean column
      when there is more than one validation on the same attribute with different scopes
        when a record exists beforehand, where all scopes are set
          when each validation has a different message
            accepts

Finished in X seconds (files took X seconds to load)
1 example, 0 failures

So what it's happening in the first test to make the second test to go wrong?

If we run these tests separetely will we see that the first test creates this record:
#<Example:0x000055aa97f1c010 id: 1, attr: "some valuf">

And when we try to run other alone it creates this:
#<Example:0x00007fbb0404c128 id: 1, attr: "dummy value", scope1: true, scope2: true>

Both pass!

But when we run both the second test creates this:
#<Example:0x000055aa9a305828 id: 1, attr: "dummy value">

Look, it's missing the scope1 and scope2. That's why the error missing attribute: :scope1. This error is happening right here after receive the scope1 as attribute_name.

One easy fix for this is to change the model name used in the second test, after that the record created in the second test has all his fields (including scope1 and scope2), but I'm don't think this is a good idea. I think I need to understand more about this feature to make a proper fix.

I'll keep digging! 😄

PS: My sincere thanks to thoughbot for this video, rspec-bisect, it helped a lot to isolate the smallest possible scenario. Great video! 👍 👍 👍

[vsppedro]

I am happy to have discovered a possible solution:

def find_existing_record
  record = model.first

  if record.present?
    record.reload
  else
    nil
  end
end

So, we know that this method was returning a record from the first test and because of that it was missing the new columns created on the second test. A quick fix for this is to execute reload on this instance.

To test this solution, run this command on the master and then on this branch:

BUNDLE_GEMFILE=/path/to/shoulda-matchers/gemfiles/rails_X_X.gemfile bundle exec rspec './spec/unit/shoulda/matchers/active_record/validate_uniqueness_of_matcher_spec.rb[1:3:2:8:1:1:1,1:5:2:2:1:2:1]' --seed 7438

PS: Remember to change rails_X_X.gemfile to one of your choices.

What do you guys think about this? Or should I take a second look at this?

[mcmire]

Yeah, so it's really weird to me that this bug is happening. It's easy enough to see that data is somehow leaking to other tests, but what's not easy to see is how. The fact that in one test you see an instance of Example being represented one way vs. another tells me that the model is caching the schema for that table per test. This makes sense because 1) both tests are defining an examples table as well as an Example model in the object space and 2) there are different columns in examples in one test vs. another. Even though both the table and model should be getting undefined after the test (proof here), ActiveRecord caches schema information about a model based on what's in the database, so that whenever you make another instance of that model, it doesn't have to ask again. That schema information is kept on the class level, so it has the potential to leak from one test to another. However, as you can see in the ModelBuilder, that cache is cleared.

So in terms of your solution, conceptually, it gets close to the underlying problem, I think, but I'm still kind of confused as to why it works. Checking the source of reload, I see this line:

self.class.connection.clear_query_cache

So I wonder what would happen if after this line you inserted:

DevelopmentRecord.connection.clear_query_cache
ProductionRecord.connection.clear_query_cache

[vsppedro]

Thank you for all this information! Unfortunately, your suggestion didn't work. The error continues.

But If I add this line:

model.reset_column_information

inside define_model also fixes the tests. In this case, we are resetting the cached information about columns when a define_model occurs. We are already using this method right here:

defined_models.each do |model|
  model.reset_column_information
end

But is not as effective as inside the define_model. So, what do you think about moving this piece of code to define_model? Another positive point for this change is that the interaction will no longer be necessary.

[mcmire]

Aha! So now that you mention it, resetting the column information for a model after defining it (and right before removing it from the object space) isn't very helpful. Your solution makes perfect sense.

[vsppedro]

Ok! I'll update the commit message. I forgot to change the description. 😅

EDIT 1: Done! 👍

[vsppedro]

Hi, @mcmire, I was wondering if something is missing here or maybe you're waiting to check if the error will happen ever again.

Sorry, I'm just curious if this PR is useful or needs any changes.

[mcmire]

Hey @vsppedro, sorry about that and thanks for reminding me about this. Everything looks good here so I'll merge it in!

[vsppedro]

No problem, thanks!