Skip to content

Add RSpec/HaveAttributes cop#2158

Open
Darhazer wants to merge 6 commits intomasterfrom
have-attributes
Open

Add RSpec/HaveAttributes cop#2158
Darhazer wants to merge 6 commits intomasterfrom
have-attributes

Conversation

@Darhazer
Copy link
Copy Markdown
Member

Fixes #1057


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

@Darhazer Darhazer requested a review from a team as a code owner January 19, 2026 11:02
@Darhazer Darhazer force-pushed the have-attributes branch 3 times, most recently from 072affe to cb04fd5 Compare January 19, 2026 11:17
@bquorning
Copy link
Copy Markdown
Collaborator

This is neat, but it is not safe. I tested this cop on one of our services, and found that it auto-corrected some longer integration tests doing something like this

expect(list.length).to eq(4)
expect(Api.call.response.code).to eq(200)
Foo.update(whatever)
expect(list.length).to eq(4)
expect(Api.call.response.code).to eq(200)

into

expect(list).to have_attributes(
  length: 4,
  length: 4
)
expect(Api.call.response).to have_attributes(
  code: 200,
  code: 200
)
Foo.update(whatever)

This is obviously pseudo-code, but if necessary I can produce a more realistic snipped.

@Darhazer
Copy link
Copy Markdown
Member Author

Thank you. I also was playing with a real code base and found out the case when there could be two expectations on the same attribute, generating a duplicated key. While this could be corrected to a combined matcher like:

expect(list).to have_attributes(
  length: eq(4).and(eq(4))
)

there is the case of an action between the expectations (which is probably what you were pointing out) that would change the result, so in a sense the corrected code would be

expect(list).to have_attributes(
  length: eq(4).and(eq(5))
)

which could never pass - it is expected to have different value in different points of time.
Should we mark it as unsafe auto-correct then?
Or perhaps to make it safer, it should work only on sequential expectations with no code that might be causing side effects in between? So it will not flag your example, but it still could group the expectations if before the Foo.update there were 2 expectations on the same object set?

@bquorning
Copy link
Copy Markdown
Collaborator

there is the case of an action between the expectations (which is probably what you were pointing out) that would change the result

Yes, this is what I was trying to point out. So as you also mention, to work safely, the cop can unfortunately only point out expectations right after each other, since it will be hard/impossible to detect if code in between the expectations could alter the result.

@Darhazer Darhazer marked this pull request as draft January 19, 2026 12:32
@Darhazer Darhazer marked this pull request as ready for review February 18, 2026 19:19
@Darhazer Darhazer mentioned this pull request Feb 20, 2026
13 tasks
@Darhazer
Copy link
Copy Markdown
Member Author

@bquorning I have updated the cop to ignore non-consecutive expectations. I'm yet to check for false negatives against a real code base, but seems the false positives are fixed and I no longer have cases when the specs are broken due to the auto-correction. Would appreciate another look / test against your code base

@bquorning
Copy link
Copy Markdown
Collaborator

Sorry for the delay. I ran this new cop on my code base and found no serious problems. I do have two comments, though:

  1. When used with Rails, date comparisons don’t work exactly the same in eq and have_attributes. E.g.
    expect(foo.created_at).to eq("2025-01-27T15:53:13Z")
    ends up in Time#== but
    expect(foo).to have_attributes(
      created_at: "2025-01-27T15:53:13Z"
    )
    ends up in String#==.
  2. It would be nice to also treat be_nil, be(nil), be(true), and be(false) as attribute checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cop idea: HaveAttributes

2 participants