Increase rails integration tests

[vsppedro]

Adding more rails integration tests similar to those in Shoulda.

The lists below track which tests are missing.

ActiveModel Matchers:

• allow_value
• have_secure_password
• validate_absence_of
• validate_acceptance_of
• validate_confirmation_of
• validate_exclusion_of
• validate_inclusion_of
• validate_length_of
• validate_numericality_of
• validate_presence_of

ActiveRecord Matchers:

• accept_nested_attributes_for
• belong_to
• define_enum_for
• have_and_belong_to_many
• have_db_column
• have_db_index
• have_implicit_order_column (Rails 6 +)
• have_many
• have_many_attached (Rails 5.2 +)
• have_one
• have_one_attached (Rails 5.2 +)
• have_readonly_attribute
• have_rich_text (Rails 6 +)
• serialize
• validate_uniqueness_of

ActionController Matchers:

• filter_param
• permit
• redirect_to
• render_template
• render_with_layout
• rescue_from
• respond_with
• route
• set_session
• set_flash
• use_after_action
• use_around_action
• use_before_action

[vsppedro]

For now, the tests are only passing on Rails 6.

TODO:

For Rails 5.2 it's necessary to prevent run_rake_tasks!('action_text:install:migrations') command to be executed and disable the matchers tests for:

• have_implicit_order_column
• have_rich_text

For Rails 5.1 or below it's necessary to prevent the same things on 5.2 plus run_rake_tasks!('active_storage:install:migrations') and disable these tests as well:

• have_many_attached
• have_one_attached

[vsppedro]

I wold like to suggest to move the rails_application.rb, rails_version.rb and load_environment.rb to outside of unit folder and use them both for unit tests and acceptance tests.

It seems to me that these three files should be shared to prevent duplications on the configuration of the project. For example this one for the migrations of the rails project:

if rails_gt_5? && bundle.includes?('actiontext')
  run_rake_tasks!('action_text:install:migrations')
end

The unit tests already have this configuration, but acceptance tests don't.

You may be wondering how I got this idea. Or maybe why.

I tried to include the rails_version.rb for the acceptance tests to be able to use methods like rails_gte_5_2? like the example above, but then I got this error: 'Rails is not defined'.

Ok. So Rails is defined on unit_spec_helper.rb, but not on acceptance_spec_helper.rb.

I don't think I should add something like a require 'rails', so I tried to understand what was going on.

The unit_spec_helper.rb first line told me where to look:

 `require_relative 'support/unit/load_environment'`

Then these three lines from load_envirohment.rb caught my attention:

$test_app = UnitTests::RailsApplication.new
$test_app.create
$test_app.load

And for my surprise, the load method is already doing almost everything that I need:

def load
  load_environment

  if rails_version > 5 && bundle.includes?('actiontext')
    add_action_text_migration
  end

  run_migrations
end

It's only missing this piece of code:

...
if rails_version >= 5.2 && bundle.includes?('active_storage')
  add_active_storage_migration
end
...

def add_action_text_migration
  fs.within_project do
    run_command! 'bundle exec rake active_storage:install:migrations'
  end
end

Should I refactor this in another PR? Or should I leave as it is?

[mcmire]

Hey @vsppedro. Sounds like you're finding some good stuff. You're right that there are some differences in how the unit tests set up the Rails applications vs. how the acceptance tests do it. This happened because I added both separately and didn't realize they were doing similar things. Since then, I've created a gem called Snowglobe which encapsulates the work of creating a Rails application for use in tests. This gem actually came out of both shoulda and shoulda-context, but it was heavily based on UnitTests::RailsApplication here. So, ideally, the test setup in shoulda-matchers could be refactored to make use of Snowglobe and that would solve the inconsistency problem you're seeing going forward. It wouldn't be a fast job*, but it would unify the two things.

I think it's fine to include changes to the unit and/or acceptance test setup in this PR if it helps you write these tests better. In terms of whether or not to do the Snowglobe refactor, if synchronizing the two manually like you're doing isn't too much trouble, maybe it's best to stick with that approach for now and we can address the refactor in a future commit.

* Snowglobe doesn't do everything that the shoulda-matchers test setup does, so it would likely have to be modified so that shoulda-matchers can make use of it. Also, in unit tests, the Rails application is loaded once into the same Ruby process in which the tests are running before all unit tests are run, whereas in acceptance tests, the Rails application is loaded within a completely separate Ruby process, for each acceptance test run. It's possible to use Snowglobe's RailsApplication class in both cases, but that distinction just has to be kept in mind when doing the refactor.

[vsppedro]

Thank you for sharing your thoughts with me.

I am interested in doing this factoring using Snowglobe, but I admit that there is still a long way to go to achieve this and I already promised other PRs here. :)

So, I will follow your suggestion and leave this refactoring for later.

For this PR, I see only the need to move rails_versions.rb to a common folder for unit tests and acceptance tests. On the last commit I moved this file to the tests folder, but if you think I should leave it somewhere else, just let me know.

[mcmire]

@vsppedro I think moving rails_versions.rb to tests makes sense — that's where I've been keeping things that are common to all kinds of tests. I made a small comment above but it looks like you're on the right track :)

[vsppedro]

I see know a better approach to this PR. I think it will help to make it leaner. :)

Let me describe what I did first and what I intend to do know, after that, I would like to hear your thoughts on this, please. When you have the time.

I've found three test scenarios so far:

1 - The rails version is equal to 6. In this case it will test all the matchers.
2 - The rails version is equal to 5.2. The matchers added for Rails 6 will be ignored: have_implicit_order_column, have_rich_text.
3 - The rails version is below 5.2. The matchers added for Rails 5.2 and 6 will be ignored: have_implicit_order_column, have_rich_text, have_many_attached, have_one_attached.

To make this happen I create two more models: user.rb and photo_album.rb with migrations, rspec and minitest.

In case of scenario 1, all the files would be added. In case os scenario 2, it would only add user.rb and photo_album.rb. In case of scenario 3, it would only add user.rb. Of course, for each scenario, the necessary migrations and test files were also added. I thought that this type of organization would be an easy way to separate the configuration for each scenario and make the code simpler.

Now I don't like that idea very much because and I think I found a better answer: def comment_lines_matching.

So, I intend to remove the post.rb and photo_album.rb models. Move the code that was in them to user.rb and use the method I discovered to dynamically comment lines that are not compatible with the version of Rails being tested.

What do you guys think?

EDIT: Another idea would be to rename user.rb to matchers_below_5_2.rb, photo_album.rb to matchers_of_5_2.rb and post.rb to machers_above_5_2.rb. Makes more sense. I'll let this for later.

PS: For now, I'm stuck in a test, I will share more information after I understand why it is failing.

[mcmire]

@vsppedro Sorry I just saw your latest comment. Sounds like you're on a good track here!