Update allow_value matcher document

[hanachin]

[vsppedro]

Hi, @hanachin, thank you for your contribution.

Can you please tell me what motivated you to create this PR?

At first I thought that the actual example was incorrect and you're sugestion the correction of it. To be sure about that I created a simple project using both examples. Both pass - but you opened my eyes to a strange behavior when using invalid values. I'll take a look on that later.

What I would like to know, please, is whether this PR would be a personal opinion of what the example should be to make its use clearer? Or is there another reason?

Thanks!

[hanachin]

@VSPPedro

I ran the example of document of allow_value matcher. It passed, but I was confused. Because the message passed to the with_message matcher is different from actual error message.

I tried some patterns.

Original example passed, because it's valid value for the sports_team.

# RSpec
RSpec.describe UserProfile, type: :model do
  it do
    should allow_value('Broncos', 'Titans').
      for(:sports_team).
      with_message('Must be either a Broncos or Titans fan',
        against: :chosen_sports_team
      )
  end
end

I changed the value that pass to the allow_value matcher to invalid one. The example passed, because the error message is different from actual error message.

# RSpec
RSpec.describe UserProfile, type: :model do
  it do
    should allow_value('foo', 'bar').
      for(:sports_team).
      with_message('Must be either a Broncos or Titans fan',
        against: :chosen_sports_team
      )
  end
end

I updated the expected message to actual one, then example failed.

# RSpec
RSpec.describe UserProfile, type: :model do
  it do
    should allow_value('foo', 'bar').
      for(:sports_team).
      with_message('Must be either a Broncos fan or a Titans fan',
        against: :chosen_sports_team
      )
  end
end

In my personal opinion, use allow_value with with_message qualifier in should is easy to make mistake. So I updated the document to fix typo in the error message, and use should_not instead of should.

	passed	failed
should allow_value(ok).with_message('expected')	x
should allow_value(ok).with_message('typo expected')	x
should allow_value(ng).with_message('expected')		x
should allow_value(ng).with_message('typo expected')	x
should_not allow_value(ok).with_message('expected')		x
should_not allow_value(ok).with_message('typo expected')		x
should_not allow_value(ng).with_message('expected')	x
should_not allow_value(ng).with_message('typo expected')		x

For testing the valid value, I want to specify the against for the allow_value matcher without error message like following:

  it do
    should allow_value('foo', 'bar').for(:sports_team).against(:chosen_sports_team)
  end

Currently I do the following.

  it do
    should allow_value('foo', 'bar').for(:sports_team).with_message(/.*/, against: :chosen_sports_team)
  end

[mcmire]

A few thoughts here:

• It's very possible that the example in the documentation doesn't work. I admit that I didn't try to run it when I wrote it, I just based it off of how I understood the matcher to work. So if this is inaccurate, then I apologize :)

• Whether you use should or should_not depends on what kind of test you want to run. When you use should you're saying "I expect that when this value is set on the attribute, the record should be valid" and when you should_not you're saying "I expect that when this value is set on the attribute, the record should be invalid". So I think it's okay to use should in the documentation, provided that that works.

• In terms of the expected behavior, I agree with @VSPPedro here. The fact that should allow_value(ok).with_message('typo expected') passes is very suspicious to me. That tells me that perhaps when using should, we are not actually checking the message itself, which seems weird. I feel like if I were using shoulda-matchers for the first time, I would expect the following scenarios (note: assume the model has a validation which runs errors.add(:attr, 'message') if not given a valid value):

  Test	Interpretation	Expected result of test
  should allow_value(valid_value).for(:attr).with_message('message')	record should not have message "message" on :attr if :attr set to valid_value	Passes
  should_not allow_value(valid_value).for(:attr).with_message('message')	record should have message "message" on :attr if :attr set to valid_value	Fails
  should allow_value(invalid_value).for(:attr).with_message('message')	record should not have message "message" on :attr if :attr set to invalid_value	Fails
  should_not allow_value(invalid_value).for(:attr).with_message('message')	record should have message "message" on :attr if :attr set to invalid_value	Passes

  Then again, it's very possible that it's been so long since I've touched this code that I don't remember exactly how this matcher should work. Or, it's possible that this matcher has been broken for a long time. Just trying to offer some explanations. 🤷‍♂️

• Finally, we are considering changing allow_value to validate_attribute which might clarify what with_message is doing exactly, so if we decide that what allow_value is doing right now is actually correct, then validate_attribute might communicate that the "message" in this case actually represents the message you receive if by chance the record is NOT valid. See https://github.com/thoughtbot/shoulda-matchers/issues/1022.

[vsppedro]

Thank you, @hanachin, for all these details. Helps a lot.

The fact that should allow_value(ok).with_message('typo expected') passes is very suspicious to me.

For me, this is unexpected behavior. I expected the matcher to fail. If we agree that this behavior is wrong, I am willing to investigate the cause and see if I can find a solution.

So I think it's okay to use should in the documentation, provided that that works.

I agree.

[matsales28]

I'd say we can close this PR. I tested a couple of scenarios and couldn't reproduce the matcher passing when an expected message with a typo. Feel free to re-open it if you see fit.