@Flink this is an interesting case, I cannot use this shoulda matcher:

it { is_expected.to validate_numericality_of(:id).is_greater_than(0).only_integer }

For testing this contract:

params do
  attribute :id, :integer

  # Negative theme IDs are for system themes only, which cannot be destroyed.
  validates :id, presence: true, numericality: { only_integer: true, greater_than: 0 }
end

ChatGPT says this is a known limitation of shoulda-matchers, since we are casting the id to an integer, when it tries to send "abcd" to test that the numericality validation works, it fails with this error:

Expected Themes::Destroy::Contract to validate that :id looks like an
integer greater than 0, but this could not be proved.
 In checking that Themes::Destroy::Contract allows :id to be ‹"abcd"›,
 after setting :id to ‹"abcd"› -- which was read back as ‹0› -- the
 matcher expected the Themes::Destroy::Contract to be invalid and to
 produce the validation error "is not a number" on :id. The record was
 indeed invalid, but it produced these validation errors instead:

 * id: ["must be greater than 0"]

 As indicated in the message above, :id seems to be changing certain
 values as they are set, and this could have something to do with why
 this test is failing. If you've overridden the writer method for this
 attribute, then you may need to change it to make this test pass, or
 do something else entirely.

AI recommends to manually test like this, but I was wondering what your thoughts are? We don't use validate_numericality_of anywhere else.

 it "is invalid when id is negative" do
  contract.id = -5
  contract.validate
  expect(contract.errors[:id]).to include("must be greater than 0")
end

@Flink except for the last couple of questions I have I've fixed up everything else, can you please take another look? 🙏

@martin-brennan regarding testing numericality when casting to an integer, I had the exact same problem in one of my PRs I’m working on. I solved the problem using allow_values, this allows to keep one-liners:
https://github.com/discourse/discourse/pull/32884/files#diff-3d10b86f5ff2b7f2b7252f3b7b3666fcd615a70f82d556d25e5cc5732e42874aR6-R8

@martin-brennan regarding testing numericality when casting to an integer, I had the exact same problem in one of my PRs I’m working on. I solved the problem using allow_values, this allows to keep one-liners

@Flink but how does that work for an ID? Allowed values are 1 or more up to the ID integer limit, how do you represent this with it { is_expected.to allow_values }?

Okay @Flink I think we are pretty much there now except for 1 or 2 last things, wdyt now? Would be good to merge this soon

@Flink but how does that work for an ID? Allowed values are 1 or more up to the ID integer limit, how do you represent this with it { is_expected.to allow_values }?

Like I did here: https://github.com/discourse/discourse/pull/32884/files#diff-3d10b86f5ff2b7f2b7252f3b7b3666fcd615a70f82d556d25e5cc5732e42874aR6-R8

You need to combine several examples to prove various things. Typically, you need at least two: one to prove which values are authorized (lowest and highest boundaries) and another one to prove that values outside those boundaries will be rejected (like minus one and plus one). Then, depending on if nil is allowed or not, you can add it to one of those one-liners.
If we have a custom validation, it’s then possible to add other lines of allow_values to easily check the outcome.
And if using a one-liner in the form of it { is_expected.to … } sometimes seems a bit obscure, nothing prevents us from adding a description or simply putting a context around the example/examples.

(I’m also adding some comments where we can use those shoulda matchers, with concrete examples)

Phew thanks again @Flink , all done now and I will merge next week 🚀

Spec failure unrelated