Shoulda-matchers: validate_uniqueness_of not using given record

[tylerhunt]

With an example like this:

specify { 
  my_record = build(:record, with_custom: :attributes)
  expect(my_record).to validate_uniqueness_of(:a_unique_field)
}

I'm currently seeing failures, since my test suite is old and broken and sometimes database records linger. The validate_uniqueness_of matcher currently checks for an existing record in the database, even if a @given_record is specified as is the case here.

I can't say if this is really a bug or not, but I'm at least looking for some explanation on the reasoning behind the matcher code. As it stands right now, if I want to ensure my given record is used, I have to manually clear all the records from the model's table before making the expectation.

This strikes me as odd behavior. If I'm going to the trouble of specifying a given record manually, I'd hope it would be used. I understand it isn't obvious how to distinguish between a given record and an implicit subject, but the current functionality is unintuitive and caused me a bit of trouble.

[mcmire]

@tylerhunt Hey, sorry about the delay in responding. I don't have a great answer for why the current behavior is the way it is exactly. My best guess is: "for historical reasons". shoulda-matchers was originally designed in the days when Test::Unit (and fixtures) were king. Perhaps because these fixtures were loaded into the database before each test it was natural to rely on their presence for tests like these. Whatever the reason, for a long time we asked people to use this matcher like so:

it do
  MyModel.create!(...)   # or use FactoryGirl
  should validate_uniqueness_of(:whatever)
end

Technically, this style is the documented way to use this matcher, but obviously we also support passing an explicit subject and we even have tests for that.

So I'm in agreement with you the current behavior doesn't need to be like that. The only thing I'm worried about is, since shoulda-matchers still supports Minitest (or is supposed to, anyway), it might be fairly far-reaching to make a change like you're proposing. I'm not sure if there are people that still rely on this behavior or not. Obviously, we could bake it into the upcoming 4.0 release, but it still seems kind of risky to me what with the other changes that we are including.

In regard to your lingering records, would it be too much trouble to add database_cleaner to your test suite?

[tylerhunt]

The app is already using database_cleaner, but there are some cases where data is created in before :all blocks and isn't being cleared out by the transactional strategy. I'm working towards doing away with both the before :all calls and the database_cleaner usage, though. This issue should resolve itself once I get to that point, but I wanted to report it here anyways since it was unexpected behavior when I encountered it. Hopefully something can be done about this in a future release. Thanks for the response, @mcmire.

[Thor77]

This is still an issue. Any news on the decision?
Explicitly specifying an instance is even recommended in the docs but doesn't work as expected because as soon as another instance of the model was already persisted that one is used instead of the given one.

[mcmire]

Thanks for reminding me about this, @Thor77. (I can't believe it's already been a year since this was posted.) I thought about it again and I feel like we could probably bake this into a patch release after we release 4.0. Even thought it's technically backward-incompatible, as you say, the behavior documented doesn't even work anyway, so I don't feel like we would be surprising people (and if they're relying on the current behavior, that behavior should still work as-is even if we fix this). I don't have a guarantee on when this will be fixed, but it's at least a legitimate bug we can certainly address.

[mcmire]

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!