Started on ActiveStorage matchers

[StefSchenkelaars]

I am working on issue #1102 to add matchers for have_one_attached and have_many_attached. I would like to receive some of your opinions about the structuring of the tests. Since the methods define some ActiveRecord associations, a scope, a callback and two methods. I've tried to cover all of the created methods however I don't want to start testing ActiveStorage itself.

[mcmire]

Hey @StefSchenkelaars. Sorry it took me so long to get back to you on this. I think this is a great start! One thing I would do is take a look at this version of association_matcher_spec.rb for an updated representation on how these tests should look. I'm working on cleaning up all of the association matcher tests and this is more the style I'm shooting for. I added a match_against matcher not too long ago, and I think you could use this to test for positive and negative failure messages more easily instead of using aggregate_failures as you're doing now.

So besides the comments I left below, I think some other things I'd want to see are:

• More tests for the different failure states you are checking in matches?
• Negative versions of these matchers. This is something we don't have currently for association matchers, but it's logic for what happens when you say should_not have_many_attached :whatevers and it essentially consists of adding a does_not_match? method as a companion to matches?. It actually get can a bit tricky because you have to think through all the failure states of this too.

[StefSchenkelaars]

Hi @mcmire,

Thanks for the review, but before I start working on it let's get some things straight. Especially the negative versions are quite interesting:

• Negative versions of these matchers. This is something we don't have currently for association matchers, but it's logic for what happens when you say should_not have_many_attached :whatevers and it essentially consists of adding a does_not_match? method as a companion to matches?. It actually get can a bit tricky because you have to think through all the failure states of this too.

When do you expect the spec to fail? Let's say you have the should_not have_many_attached :photos but in the model you did define a method def photos. I personally think that the test should just pass and only if all the conditions are true (so it has photos, photos=, etc), the spec should fail. What about you?

[StefSchenkelaars]

Hi @mcmire, I started on refactoring the specs towards the match_against matcher. However with the current implementation of this matcher, I can't make the difference between a false positive or a false negative. Let me give an example:

This is the case when you do want to check if the model has an attached avatar (should have_one_attached :avatar). However it fails because the blobs relations is not set correctly.

context 'and the blobs association is invalid' do
  it 'matches' do
    record = record_has_one_attached(:avatar, invalidate_blobs: true)

    expect { have_one_attached(:avatar) }.
      not_to match_against(record).
        and_fail_with <<-MESSAGE
Expected User to have a has_one attached called avatar, but this could not be proved.
  Expected User to have a has_one association called avatar_blob (avatar_blob should resolve to ActiveStorage::Blob for class_name)
    MESSAGE
  end
end

This is the case where you want to check should_not have_one_attached :avatar:

context 'when the attached does not exsist on the model' do
  it 'matches' do
    model = define_model('User') {}
    record = model.new

    expect { have_one_attached(:avatar) }.
      to_not match_against(record).
        and_fail_with <<-MESSAGE
Did not expect User to have a has_one attached called avatar, but it does.
    MESSAGE
  end
end

Both of these cases use the to_not match_against and thus use the does_not_match? of my matcher. This way the matcher cannot determine what failure message to show. So how do you see this? Change the match_against matcher to allow to match_agains(record).and_fail_with? This will now raise an error but it is basically what the first case should be. It matches against a record but it fails because it's false. You agree?

[hound]

Some files could not be reviewed due to errors:

cannot load such file -- rubocop-rails

cannot load such file -- rubocop-rails
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `require'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `each'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:43:in `load_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_loader.rb:83:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:181:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.54.0/lib/rubocop/cli.rb:40:in `run'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.54.0/bin/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

[gmmcal]

I was planning on starting this feature. Looking forward for using this.

[trcarden]

@StefSchenkelaars / @mcmire is this still in progress or did it stall out? Looks like its ready for review and would be a nice addition.

[mcmire]

Yup, thanks for reminding. I'm not immediately sure what's being asked above so I'll take another look at this.

[StefSchenkelaars]

@mcmire I noticed that the issue was closed and that this PR has been stale for some time. I just rebased on master so the merge conflicts are out and IMO it's still in good enough shape to be merged. If there is anything you want to be changed, let me know, then I can have a look again 👍

[mcmire]

Hey, sorry this has just been sitting here. I took another look through this. Overall this is looking good, I just had a few more suggestions if you don't mind and then we should be good to go on this.

[mcmire]

Just a style thing, but do you mind adding an empty line around before and after it do ... end? Same goes for context do ... end.

[StefSchenkelaars]

Sure. Maybe an idea to add some rubocop checks for this kind of stuff?

[mcmire]

Yeah, I had Hound enabled at one point but it doesn't seem to be working well anymore. I'll look to see if there is a new GitHub Action that someone has made that does the same thing.

[mcmire]

@StefSchenkelaars By the way, I just read back through your comments around the negative matchers. I think what you have here is probably fine, so I'm not too concerned about it. Negative versions of matchers are rarely used and I'm sure if someone discovers an issue then they will raise it and we can tweak this further. But from a big picture, this is good as a first version.

[StefSchenkelaars]

@mcmire I've updated the code. I also noticed that the test suite was failing this time since of a foreign key constraint. I saw the structure of the creating and deleting of the tables and I thought the easiest thing to do is to just remove the foreign key since it doesn't affect the matcher nor the tests.

[mcmire]

Thanks so much for this — this looks good to me now! Would you like to squash your commits before I merge? I'm happy doing it as well, just wanted to give you the opportunity in case it matters to you.

[StefSchenkelaars]

@mcmire Doesn't matter too much to me but it's always nice to have the correct author on the commit. So unless CI now suddenly fails, you are free to merge it 👍

[mcmire]

Great! Thank you!

[ruvaleev]

Execuse me, I see, that PR already merged, but my shoulda-matchers still trhrows error about have_one_attached matcher (

    Failure/Error: it { should have_one_attached :file }
       expected #<Attachment:0x00007fd21ea569f8> to respond to `has_one_attached?`

). I've try to remove version specification from Gemfile (change gem 'shoulda-matchers', '~> 4.0' to gem 'shoulda-matchers'), and even run gem update 'shoulda-matchers', but nothing has changed. This feature is already merged to production? And how can I use it? Or we should to wait new release or something?

P.S.: Thanks!

[gmmcal]

@ruvaleev this feature is still unreleased. You can see it on CHANGELOG.md. You can either point your dependency to master and have this feature or wait for it to be officially released.

[ruvaleev]

@gmmcal Ok, thanks )