allow_value should detect if values are changed post-validation

[mcmire]

A colleague at work just ran into #836, except the scenario is different. Basically, Devise adds a before_validation on whatever field you're using for authentication (let's say username) so that the value this is set to will be downcased. So this screws up the uniqueness matcher in a similar manner to what's posted in that issue.

Unfortunately, the matcher isn't detecting this and so you get a nebulous failure message from the uniqueness matcher that "A" was set on the record and an error message was found on "a". It'd probably be worth it to add some kind of check after we call valid? to ensure that all of the values that we've set on the model remain the same post-validation.

[dgmstuart]

Perhaps I've misunderstood, but for me, whether the values pre- and post-validation are the same isn't something I care about when validating uniqueness: in some cases I'll explicitly want to change values as part of validation, so I want them to be different but it may not be a property of my code that I'm interested in making assertions about.

Of course, in some situations I may want to explicitly assert this property, but it's odd to me that shoulda-matchers would be asserting it in an apparently unrelated check: from my perspective this feels like part of my test suite is deciding to care about properties that I don't care about, in a way that I can't control.

Or are we saying that shoulda-matchers is only applicable if you never change values in-between setting and getting? I guess that would be a pretty clear and coherent constraint, but it would be a little sad as well.

[mcmire]

The goal I am trying to achieve is to provide more informative and helpful error messages, and to eliminate confusion when a matcher fails.

Currently, allow_value will raise an exception if the value that the matcher puts into the attribute under test isn't the same as the value coming out of the attribute. This behavior may be intentional, but it changes the test being run, and can lead to unintuitive test failures (which I've personally witnessed).

In #836, a similar thing occurs, except the value the attribute stores doesn't change immediately, it changes when validations are run. Here's an example of the model:

class User
  before_validation :downcase_username

  validates :username, uniqueness: true

  private

  def downcase_username
    self.username = username.downcase
  end
end

If you test this using shoulda-matchers:

describe User do
  it { should validate_uniqueness_of(:username) }
end

this test will fail with something like:

Expected not to have errors on :username when value was set to "A", got errors:

* "has already been taken" (attribute: :username, value: "a")

This is a very confusing error message. The matcher should "just work", right?

Except it doesn't. Why? Essentially, the matcher is trying to make an assertion to test the case-sensitivity behavior of the validation, and the fact that username is being downcased is preventing the matcher from doing so. Now, we can add another qualifier to the uniqueness matcher to tell it not to do this, but that's not the point here -- the point is that instead of receiving a failure message like the above, what you should get is something like:

`validate_uniqueness_of` is attempting to assert that your attribute has a case-sensitive uniqueness
validation on :username by setting it to value of "A", but when it was read back, it had stored "a" instead.
It's likely that :username is overridden and is interfering with the test. If the writer for :username is
changing the case of incoming values, try using the `ignoring_case_sensitivity` qualifier.

Now this implies a narrower strip of logic -- whether username changes the case of incoming values, and not that it changes it to some arbitrary value. So obviously, we could have the uniqueness matcher perform just this check.

But, I think any time you have an attribute that changes incoming values in any way, it may not be bad from the perspective of your application, but it's bad from the perspective of the gem, because the gem pretty heavily relies on the fact that values are going to stay the same.

So yes, it's sad that we have to put in these restrictions, but at least that way you'll get a very specific error message for a very specific reason that I can help you with, rather than potentially receive a failure message that I'll have to spend time to debug.

[dgmstuart]

I had indeed misunderstood: if we are indeed going to implement :ignoring_case_sensitivity then it makes a lot of sense to have this sort of message :)

This is great - it reminds me of the sort of "did you mean" features that Matz is talking about for Ruby 3.

Are you already implementing ignoring_case_sensitivity, or is that something I can take off your plate?

[dgmstuart]

My concern was because I thought you were saying that we should ONLY give you the error message (and no way to deal with it).

[mcmire]

My concern was because I thought you were saying that we should ONLY give you the error message (and no way to deal with it).

Yup, that's a good point.

I haven't started implementing ignoring_case_sensitivity, so if you want to get started on it, that would be great :)

[dgmstuart]

Consider me on it

[dgmstuart]

@mcmire So I've made a good start and seems to me like there are a few distinct steps:

1. Add specs covering the e.g. "override setter to upcase the value" cases, asserting that the current behaviour happens
2. Refactor the existing code to take symbols instead of booleans (e.g. options[:case] = :sensitive instead of options[:case_insensitive] = false)
3. Add the new behaviour (A method: ignoring_case_sensitivity, which sets the option options[:case] = :ignore

A few questions:

1. Does this sound sensible?
2. https://github.com/thoughtbot/shoulda-matchers/pull/820 covers some of the spec part - should I base my branch off that PR instead of off master?
3. I haven't contributed to a project of this scale before so I'd appreciate a steer: is it preferable for these to be separate PRs, or is it sufficient for them to be distinct commits?

[mcmire]

Fixed by #840.