@═╦╣╚╗ A mazing engineer 30-Jun-2019 Fixing RSpec/SubjectStub Cop One of my favourite open source projects is (I know, this is rather strange) rubocop-rspec. I won't dive deep into why having correct and readable tests is important. rubocop-rspec is enforcing RSpec Style Guide, of which I'm one of the editors. It is providing many more options to enforce consistency of the code style and find common flaws in your project's test code. The guide and rubocop-rspec are, unfortunately, not completely in sync, but eventually they will be. I've started to actively submit to this repository just recently. My second cop happened to grow to a monster I'll have to split up into several cops. There were 80+ open issues at the time with a growing trend. In an attempt to bring everything in order and reduce the burden of having such a lengthy TODO (very well described in Always Be Closing), I started addressing issues one by one. Next ticket was Inconsistent behaviour of SubjectStub. The idea behind the cop is described in detail by Sam Phippen in RSpec Smells: Stubject, and ThoughtBot's Don't Stub the System Under Test. The details of the bug report were:

    # detects offences
    expect(subject).to receive(:my_method)
    expect(subject).to receive(:my_method).and_call_original
    expect(subject).to receive(:my_method).and_wrap_original { |m, *| m.call(42) }

    # does not detect an offence
    expect(subject).not_to receive(:my_method)

I imagined the fix will be trivial, something like:

    - :to
    + { :to :not_to :to_not }

However, I've immediately spotted a number of other flaws in the cop, and couldn't resist fixing them all. On my daily job, I would normally file a ticket flagged as tech-debt, and would address it at the end of the sprint in that spare time one can dedicate to any desired improvements. But with no deadlines, nor sprint constraints, no disturbing thoughts about scope creep, it's so natural to take a thing and make it as good as it can be. First off, cop's spec example context descriptions were inconsistent, mixing "complains" with "flags". I've seen different variations, but "flags" and its opposite, "ignores" seem to be the most commonly used. Secondly, it turned out that one-liner expectation syntax was not properly detected. Also, allow(subject) without .to receive(...) would be flagged as well. It's valid syntax, but without a matcher attached it makes no sense. I've seen a worse case several times in the wild, when matcher is used on its own, as a complete statement, without expectations, e.g. validates_presence_of(:email) without is_expected.to. There's not yet a cop for it. Next, the original issue with negated runners, not_to and to_not. Apart from receive, RSpec provides receive_messages and receive_message_chain. Adding support for them was as trivial as for negated runners. Additionally, a spy-style message expectations (.to have_received). Offence message was not very self-explanatory, and it's something that takes a lot of time and is the cause of major frustration when those offences come up. Could not leave a non-descriptive message. Added references to regarded sources. A corner case when a named subject is defined, but the expectation is made on a literal subject. It was all quite straightforward up until that moment. And then came the trouble. There was a special case in the cop to ignore the all matcher being applied to the subject:

    expect(subject).to all receive(:baz)

It is still stubbing the subject. Why was this introduced? Turns out, to ignore the offence that the cop would raise in the following test code in rubocop:

    expect(formatter_set).to all receive(:started).with(files)

Originally, this test code was introduced to appease another cop, RSpec/IteratedExpectation. And that change has added a # rubocop:disable RSpec/SubjectStub directive. And later a quick fix to rubocop-rspec was made to ignore all matcher when used with expect(subject). This case in rubocop test code is quite interesting. There's a formatter_set, containing entries, formatters, and delegating specific method calls to them. As an additional puzzle, there's no way to add entries to the set, except by passing their attributes. That resulted in the following test code:

    formatter_set.add_formatter('simple')
    formatter_set.add_formatter('emacs')

    allow(formatter_set).to all receive(:started)

    formatter_set.started(files)

    expect(formatter_set).to all have_received(:started).with(files)

It didn't seem right to me. If I would decide to remove the all special case, I would also have to fix the rubocop test code so that on the next rubocop-rspec update there would be no offences. The change itself was trivial, but there was another problem with the new code, the default have_received enforced style of RSpec/MessageSpies. What does it mean? It means that:

    # bad
    expect(foo).to receive(:bar)

    # good
    expect(foo).to have_received(:bar)

Both are valid syntaxes, and there's only one good reason to pick the latter for the former - Arrange-Act-Assert pattern, or Four-Phase Test (setup-excercise-verify-teardown) pattern. This pattern is unfortunately commonly overlooked. rubocop itself used both syntaxes. A quick search showed 36 occurrences of to have_received and 97 of to receive. I did another check, just to make sure I'm not trying to promote a bad practice, and grep'ed through a number of Ruby project codebases conveniently gathered in one repository, Real World Ruby Apps. .to receive were even more prevailing, with 21453 occurrences over 102 for .to have_received. That inspired me to continue. Since I've checked out all submodules of real-world-ruby-apps, I decided to run a modified version of RSpec/SubjectStub cop that would only detect .to all receive on a subject. Surprisingly there was no single offence detected. Surprisingly - because rubocop is one of the Real World Ruby Apps' submodules. And it was not detected, even though I knew that on latest rubocop's master it would raise an offence. Unfortunately, this is due to how submodules work - you have to specify a tip of the linked repository, and rubocop was out of date there. I must confess, I did not update all the submodules and re-checked, only did that with rubocop to make sure my experiment is correct. It did, of course, detect an offence. To find an additional proof, I've checked out a similar Real World Rails repository, that incorporates a fair number of applications written in Rails and pluggable Rails engines. No single offence there as well. It was the only offence in the popular open sourced Ruby projects. And this confirmed the information that the special case of dealing with expect(subject).to all receive was introduced to deal with the only offence in a single repository. So, I've sent a pull request to switch RuboCop's own test code to receive completely. It was accepted almost immediately, probably partially due to a detailed pull request description, you may want to read it. If you follow the comments, there's my explanation why I did not (at least in most cases) break the A-A-A pattern. Code-wise I see this as an improvement, I've spent quite some time on this change to make sure the tests are at least as readable as previously. This commit was the most important, with this change being merged I had a weighty argument in rubocop-rspec's pull request for removing the special case for all matcher. RSpec/SubjectStub, RSpec/IteratedExpectation, and RSpec/MessageSpies were all appeased. Yay! I've added another commit to my rubocop-rspec pull request to get rid of this special case. I still had to convince rubocop-rspec maintainers though that all matcher is still technically stubbing the subject itself. Not going to quote my explanation, check it out if you are interested in the details. We all agreed that it's ok to remove the special case. There's still a workaround if you can't find a way around stubbing your subject that is a delegating container (notice .each):

    expect(subject.each).to all receive(:some_method)

All the changes described above went to a relatively small +130 -31 pull request that has been recently merged. I continue my work on reducing the number of issues/pulls in rubocop-rspec along with the other regular committers, but there's still a long road towards Zero Issues. Join the movement, it's pretty entertaining! Takeaways: - the practices common to repositories consisting Real World Ruby/Rails Apps can be used as an argument that a specific practice is common, and in general appreciated by Ruby community - it's all so complicated, but still fixable with enough persistence